Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem copying files when container is in host pid namespace #10327

Merged
merged 1 commit into from
May 19, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented May 13, 2021

When attempting to copy files into and out of running containers
within the host pidnamespace, the code was attempting to join the
host pidns again, and getting an error. This was causing the podman
cp command to fail. Since we are already in the host pid namespace,
we should not be attempting to join. This PR adds a check to see if
the container is in NOT host pid namespace, and only then attempts to
join.

Fixes: #9985

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we join the pidns to not break /proc?

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 13, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


func (c *Container) hostPidNS() (bool, error) {
if c.config.PIDNsCtr != "" {
return false, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, we could go further here, and call hostPidNS() on that container - it could potentially be a pid=host container that we joined the namespace of?

I don't know if this is particularly valuable though.

@TomSweeneyRedHat
Copy link
Member

@rhatdan changes LGTM, but it appears you've a gofmt error somewhere.

@rhatdan rhatdan force-pushed the copy branch 4 times, most recently from 9100e57 to e41a359 Compare May 17, 2021 19:14
@rhatdan rhatdan added the 3.2 label May 17, 2021
test/e2e/cp_test.go Show resolved Hide resolved
@@ -890,3 +890,25 @@ func (c *Container) generateInspectContainerHostConfig(ctrSpec *spec.Spec, named

return hostConfig, nil
}

func (c *Container) hostPidNS() (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read hostPidNS() like it's returning the host's PID NS.

Suggested change
func (c *Container) hostPidNS() (bool, error) {
// Return true if the container is running in the host's PID NS.
func (c *Container) inHostPidNS() (bool, error) {

libpod/container_copy_linux.go Show resolved Hide resolved
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TomSweeneyRedHat
Copy link
Member

Still fighting with the test system on this one @rhatdan

@@ -91,6 +91,49 @@ var _ = Describe("Podman cp", func() {
Expect(roundtripContent).To(Equal(originalContent))
})

// Copy a file to the container, then back to the host in --pid=host
It("podman cp --pid=host file", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make ubuntu happy.

Suggested change
It("podman cp --pid=host file", func() {
It("podman cp --pid=host file", func() {
SkipIfRootlessCgroupsV1("Not supported for rootless + CGroupsV1")

When attempting to copy files into and out of running containers
within the host pidnamespace, the code was attempting to join the
host pidns again, and getting an error. This was causing the podman
cp command to fail. Since we are already in the host pid namespace,
we should not be attempting to join.  This PR adds a check to see if
the container is in NOT host pid namespace, and only then attempts to
join.

Fixes: containers#9985

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label May 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4683225 into containers:master May 19, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Container created with --pid host returns operation not permitted on copy in F34
6 participants