Do not mount sysfs as rootless in more cases #8561

mheon · 2020-12-02T20:04:03Z

We can't mount sysfs as rootless unless we manage the network namespace. Problem: slirp4netns is now creating and managing a network namespace separate from the OCI runtime, so we can't mount sysfs in many circumstances. The crun OCI runtime will automatically handle this by falling back to a bind mount, but runc will not, so we didn't notice until RHEL gating tests ran on the new branch.

edsantiago · 2020-12-02T20:09:23Z

Just curious: don't the Ubuntu CI VMs use runc? I'm wondering how/if we could have caught this earlier, and how we can test that this will work in RHEL.

mheon · 2020-12-02T20:32:38Z

@giuseppe PTAL

@edsantiago That is an excellent question. I think they do. Possibly a newer version than RHEL runc, that has its own workaround for this?

mheon · 2020-12-02T21:06:39Z

Memory tests are definitely broken, they're looking at things in /sys/fs/cgroup...

giuseppe · 2020-12-03T12:39:19Z

pkg/specgen/generate/oci.go

@@ -165,7 +165,7 @@ func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runt
 			inUserNS = true
 		}
 	}
-	if inUserNS && s.NetNS.IsHost() {
+	if inUserNS && s.NetNS.NSMode != specgen.NoNetwork {


doesn't it work when the NSMode is set to private?

I suspect it doesn't because we still make the namespace in Podman in that case

I'll check again to verify.

You are entirely correct, it does work.

mheon · 2020-12-03T15:22:16Z

Re-pushed with fix.

mvollmer · 2020-12-03T18:27:34Z

Thanks for this fix! We ran into this while testing cockpit-podman on our rhel-8-4 image, which should be representative for a generic RHEL 8.4 installation.

And now I am confused about runc vs crun. What should be in use in a bone stock RHEL 8 installation, runc or crun? In other words, would a normal RHEL 8 podman user run into this bug, or not?

I ask because I am worried that we are unknowingly testing some less interesting configuration of RHEL 8.

Thanks!

mheon · 2020-12-03T18:58:32Z

The default is runc - crun only became available with 8.3.0, AFAIK, and there's no way we're making it default before 9.0

mvollmer · 2020-12-04T09:23:49Z

The default is runc - crun only became available with 8.3.0, AFAIK, and there's no way we're making it default before 9.0

Thanks, so we are doing it right!

rhatdan · 2020-12-04T11:33:07Z

pkg/specgen/generate/oci.go

@@ -165,7 +165,7 @@ func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runt
 			inUserNS = true
 		}
 	}
-	if inUserNS && s.NetNS.IsHost() {
+	if inUserNS && !(s.NetNS.NSMode == specgen.NoNetwork || s.NetNS.NSMode == specgen.Private) {


I am not sure this is correct? And I think we should be using the functions in namespaces? I am not sure if all cases are handled.

NSMode == "default"
NSMode == ""
are the same as
NSMode == "private"

No, they're not. We explicitly do not want to trigger this with slirp4netns networking, and both of those can trigger slirp.

If private also triggers slirp, then we'll need to explicitly disable it as well.

I'm going to revert back to NoNetwork. It's guaranteed to be safe. We can loosen this later if we need to.

So we need to have a check added to indicate whether or not we are using slirp4netns networking.

I'm actually not sure if CNI + user namespaces is safe either. It's probably better to leave disabled.

mheon · 2020-12-04T15:00:39Z

Re-pushed with fixes for tests - I disabled networking on a lot of tests that accessed /sys/fs/cgroup as cgroupsv2 to bypass the sysfs problem.

We can't mount sysfs as rootless unless we manage the network namespace. Problem: slirp4netns is now creating and managing a network namespace separate from the OCI runtime, so we can't mount sysfs in many circumstances. The `crun` OCI runtime will automatically handle this by falling back to a bind mount, but `runc` will not, so we didn't notice until RHEL gating tests ran on the new branch. Signed-off-by: Matthew Heon <[email protected]>

rhatdan · 2020-12-05T10:39:37Z

@giuseppe PTAL

mheon · 2020-12-07T14:50:43Z

@containers/podman-maintainers PTAL, this is needed for 2.2.1

saschagrunert

LGTM 👍

openshift-ci-robot · 2020-12-07T15:29:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, saschagrunert

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:

~~OWNERS~~ [mheon,saschagrunert]

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

giuseppe · 2020-12-07T16:35:44Z

/lgtm

giuseppe · 2021-01-12T09:32:05Z

this PR caused a regression as reported here: systemd/systemd#17902 (comment)

podman run  --rm fedora grep cgroup /proc/self/mountinfo
1893 1891 0:27 /../../../../../.. /sys/fs/cgroup rw,nosuid,nodev,noexec,relatime - cgroup2 cgroup2 rw,seclabel,nsdelegate

Do you remember what was the failure we saw without this commit?

giuseppe · 2021-01-12T13:58:48Z

@mvollmer do you have a reproducer for the issue you were seeing?

mheon · 2021-01-12T14:11:38Z

@giuseppe Was causing failures to run rootless Podman on cgroupsv1 + runc (most notably on the RHEL8 test suite, but Ubuntu and other v1 distros were also affected).

giuseppe · 2021-01-12T15:09:20Z

I've opened a PR to see if we catch any of these failures in the CI: #8949

jonasbb · 2021-01-14T10:14:26Z

@giuseppe Would #8949 also help with #8712? #8712 has steps to reproduce.

giuseppe · 2021-01-14T12:53:43Z

@giuseppe Would #8949 also help with #8712? #8712 has steps to reproduce.

yes

jonasbb · 2021-01-14T13:12:42Z

Glad to hear. Then I hope this PR or a similar one can be merged for the next release.

RHEL8 rootless gating tests are inconsistently failing with: $ podman diff --format json -l # {"changed":["/etc"],"added":["/sys/fs","/sys/fs/cgroup","/pMOm1Q0fnN"],"deleted":["/etc/services"]} # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv # #| FAIL: added # #| expected: '/pMOm1Q0fnN' # #| actual: '/sys/fs' # #| > '/sys/fs/cgroup' # #| > '/pMOm1Q0fnN' # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Reason: PR containers#8561, I think (something to do with /sys on RHEL). Workaround: ignore '/sys/fs' in diffs. Signed-off-by: Ed Santiago <[email protected]>

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020

giuseppe reviewed Dec 3, 2020

View reviewed changes

mvollmer mentioned this pull request Dec 3, 2020

podman rootless mode doesn't work cockpit-project/bots#1436

Closed

mheon force-pushed the fix_gating branch from 11ddbf5 to faa26a7 Compare December 3, 2020 15:21

rhatdan reviewed Dec 4, 2020

View reviewed changes

mheon force-pushed the fix_gating branch from faa26a7 to f7f6027 Compare December 4, 2020 15:00

mheon force-pushed the fix_gating branch from f7f6027 to a06de94 Compare December 4, 2020 18:03

mheon force-pushed the fix_gating branch from a06de94 to 95c4577 Compare December 4, 2020 18:50

saschagrunert approved these changes Dec 7, 2020

View reviewed changes

openshift-ci-robot assigned giuseppe Dec 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020

openshift-merge-robot merged commit aac03d4 into containers:master Dec 7, 2020

This was referenced Dec 14, 2020

sysfs appears to be bind mounted into containers which use non-host networking #8707

Closed

Podman 2.2.1 breaks buildah in Podman container. #8712

Closed

edsantiago mentioned this pull request Feb 2, 2021

Gating tests: diff test: workaround for RHEL8 failure #9209

Merged

edsantiago mentioned this pull request Feb 3, 2021

[v3.0] Gating tests: diff test: workaround for RHEL8 failure #9213

Merged

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 bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not mount sysfs as rootless in more cases #8561

Do not mount sysfs as rootless in more cases #8561

mheon commented Dec 2, 2020

edsantiago commented Dec 2, 2020

mheon commented Dec 2, 2020

mheon commented Dec 2, 2020

giuseppe Dec 3, 2020

mheon Dec 3, 2020

mheon Dec 3, 2020

mheon Dec 3, 2020

mheon commented Dec 3, 2020

mvollmer commented Dec 3, 2020

mheon commented Dec 3, 2020

mvollmer commented Dec 4, 2020

rhatdan Dec 4, 2020

mheon Dec 4, 2020 •

edited

Loading

mheon Dec 4, 2020

mheon Dec 4, 2020

rhatdan Dec 4, 2020

mheon Dec 4, 2020

mheon commented Dec 4, 2020

rhatdan commented Dec 5, 2020

mheon commented Dec 7, 2020

saschagrunert left a comment

openshift-ci-robot commented Dec 7, 2020

giuseppe commented Dec 7, 2020

giuseppe commented Jan 12, 2021

giuseppe commented Jan 12, 2021

mheon commented Jan 12, 2021

giuseppe commented Jan 12, 2021

jonasbb commented Jan 14, 2021

giuseppe commented Jan 14, 2021

jonasbb commented Jan 14, 2021

Do not mount sysfs as rootless in more cases #8561

Do not mount sysfs as rootless in more cases #8561

Conversation

mheon commented Dec 2, 2020

edsantiago commented Dec 2, 2020

mheon commented Dec 2, 2020

mheon commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Dec 3, 2020

mvollmer commented Dec 3, 2020

mheon commented Dec 3, 2020

mvollmer commented Dec 4, 2020

Choose a reason for hiding this comment

mheon Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Dec 4, 2020

rhatdan commented Dec 5, 2020

mheon commented Dec 7, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 7, 2020

giuseppe commented Dec 7, 2020

giuseppe commented Jan 12, 2021

giuseppe commented Jan 12, 2021

mheon commented Jan 12, 2021

giuseppe commented Jan 12, 2021

jonasbb commented Jan 14, 2021

giuseppe commented Jan 14, 2021

jonasbb commented Jan 14, 2021

mheon Dec 4, 2020 •

edited

Loading