overlay2: support "userxattr" option (kernel 5.11) by AkihiroSuda · Pull Request #42068 · moby/moby

AkihiroSuda · 2021-02-24T09:58:38Z

- What I did

Fix #42055 "[kernel 5.11 + overlay2 + rootless] : apt-get fails with Invalid cross-device link"

- How I did it

Support "userxattr" option (kernel 5.11).

The "userxattr" option is needed for mounting overlayfs inside a user namespace with kernel >= 5.11.

The "userxattr" option is NOT needed for the initial user namespace (aka "the host").

Also, Ubuntu (since circa 2015) and Debian (since 10) with kernel < 5.11 can mount the overlayfs in a user namespace without the "userxattr" option.

The corresponding kernel commit: torvalds/linux@2d2f2d7

ovl: user xattr

Optionally allow using "user.overlay." namespace instead of "trusted.overlay."
...
Disable redirect_dir and metacopy options, because these would allow privilege escalation through direct manipulation of the
"user.overlay.redirect" or "user.overlay.metacopy" xattrs.

Related to containerd/containerd#5076

- How to verify it

Install Ubuntu 20.10
Install mainline kernel 5.11 https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.11/amd64/
Launch rootless dockerd with overlay2 storage driver.
Make sure docker --context=rootless run -it --rm ubuntu sh -ec "apt-get update && apt-get install -y sl" succeeds without an error ([kernel 5.11 + overlay2 + rootless] : apt-get fails with Invalid cross-device link #42055).

- Description for the changelog

overlay2: support "userxattr" option (kernel 5.11)

- A picture of a cute animal (not mandatory but encouraged)
🐧

AkihiroSuda · 2021-03-10T05:58:24Z

Moving out of draft.

@thaJeztah @tiborvass @StefanScherer Can we have a new release with this ASAP, before people begin using kernel 5.11 next month (Ubuntu 21.04 & Fedora 34)?

The "userxattr" option is needed for mounting overlayfs inside a user namespace with kernel >= 5.11. The "userxattr" option is NOT needed for the initial user namespace (aka "the host"). Also, Ubuntu (since circa 2015) and Debian (since 10) with kernel < 5.11 can mount the overlayfs in a user namespace without the "userxattr" option. The corresponding kernel commit: 2d2f2d7322ff43e0fe92bf8cccdc0b09449bf2e1 > **ovl: user xattr** > > Optionally allow using "user.overlay." namespace instead of "trusted.overlay." > ... > Disable redirect_dir and metacopy options, because these would allow privilege escalation through direct manipulation of the > "user.overlay.redirect" or "user.overlay.metacopy" xattrs. Fix issue 42055 Related to containerd/containerd PR 5076 Signed-off-by: Akihiro Suda <[email protected]>

Signed-off-by: Akihiro Suda <[email protected]>

thaJeztah · 2021-03-11T20:23:49Z

+	if err := os.RemoveAll(tdRoot); err != nil {
+		logrus.WithError(err).Warnf("Failed to remove check directory %v", tdRoot)
+	}


Is there a reason we're removing here, but also in the defer? Is this to cleanup the test-dir from a previous daemon start?

(if so; would it be better to create a tempdir, so that we don't reuse the same directory?)

Intentionally reusing the same directory to avoid filling up the root directory with unmountable dirs. Discussed in containerd/containerd#5076 (comment)

Ah, thanks for explaining, yes, that makes sense.

I think a comment explaining would be useful (perhaps an inline cleanup() function that's called both here, and in the defer, but ok to leave for a follow-up)

cpuguy83 · 2021-03-11T20:27:19Z

+		userxattr = "userxattr,"
+	}
+
+	logger.Debugf("backingFs=%s, projectQuotaSupported=%v, indexOff=%q, userxattr=%q",


Can we remove this from the init?

This Debugf is not new in this PR.

I'm personally ok with keeping this (for now) as it's not new indeed.

Looking at the code, I also want to do some refactoring after this is merged; the string-concatting code is becoming a bit messy and, assuming that won't affect performance, I'd like to use a []string for these options, and strings.Join() them at the end (I think that will make the code more readable).

Oh this was a github UI issue, its not in the init like I thought.

thaJeztah

LGTM

thaJeztah · 2021-03-12T08:38:02Z

+
+	opts := []string{
+		fmt.Sprintf("lowerdir=%s:%s,upperdir=%s,workdir=%s", filepath.Join(td, "lower2"), filepath.Join(td, "lower1"), filepath.Join(td, "upper"), filepath.Join(td, "work")),
+		"userxattr",


We can probably combine this check with SupportsOverlay() (adding a parameter to check with userxattr set); I'll look at that in a follow-up

AkihiroSuda · 2021-03-16T06:52:13Z

@thaJeztah @tonistiigi

Can we merge this PR as-is without revendoring containerd, or do we need to revendor containerd/containerd#5204 ?

This needs to be cherry-picked into v20.10 (ASAP), so I'm not sure we want to revendor containerd.

thaJeztah · 2021-03-16T12:00:05Z

Can we merge this PR as-is without revendoring containerd, or do we need to revendor containerd/containerd#5204 ?
This needs to be cherry-picked into v20.10 (ASAP), so I'm not sure we want to revendor containerd.

yes; we should merge this with the current (copied) code, especially for that reason (we don't want to be updating containerd vendor code in the 20.10 branch, unless there's a really important reason).

After this is merged, we can look at re-vendoring containerd (I'll be opening a PR today probably, to test) for 21.xx

thaJeztah · 2021-03-16T12:00:26Z

@cpuguy83 PTAL

cpuguy83

LGTM

AkihiroSuda added area/rootless Rootless Mode area/storage/overlay process/cherry-pick labels Feb 24, 2021

AkihiroSuda commented Feb 24, 2021

View reviewed changes

Comment thread daemon/graphdriver/overlay2/overlay.go Outdated

thaJeztah added this to the 21.xx milestone Feb 24, 2021

AkihiroSuda mentioned this pull request Feb 25, 2021

overlay: support "userxattr" option (kernel 5.11) containerd/containerd#5076

Merged

AkihiroSuda marked this pull request as ready for review March 10, 2021 05:57

AkihiroSuda force-pushed the ovl-k511 branch from 4f5a76a to 2053a33 Compare March 10, 2021 05:57

thaJeztah requested changes Mar 10, 2021

View reviewed changes

Comment thread daemon/graphdriver/overlay2/overlay.go Outdated

Comment thread daemon/graphdriver/overlay2/overlay.go Outdated

AkihiroSuda added 2 commits March 11, 2021 15:12

overlayutils/userxattr.go: add "fast path" for kernel >= 5.11.0

a8008f7

Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the ovl-k511 branch from 2053a33 to a8008f7 Compare March 11, 2021 06:22

thaJeztah reviewed Mar 11, 2021

View reviewed changes

cpuguy83 requested changes Mar 11, 2021

View reviewed changes

AkihiroSuda mentioned this pull request Mar 12, 2021

[kernel 5.11 + overlay2 + rootless] : apt-get fails with Invalid cross-device link #42055

Closed

thaJeztah approved these changes Mar 12, 2021

View reviewed changes

thaJeztah mentioned this pull request Mar 15, 2021

move overlay-checks to an overlayutils package containerd/containerd#5204

Merged

tonistiigi approved these changes Mar 18, 2021

View reviewed changes

cpuguy83 approved these changes Mar 18, 2021

View reviewed changes

cpuguy83 merged commit a84d824 into moby:master Mar 18, 2021

AkihiroSuda mentioned this pull request Mar 18, 2021

[20.10 backport] overlay2: support "userxattr" option (kernel 5.11) #42168

Merged

AkihiroSuda added process/cherry-picked and removed process/cherry-pick labels Mar 18, 2021

AkihiroSuda mentioned this pull request Mar 20, 2021

overlayfs: s/trusted/user/ when userxattr is enabled (required for avoiding I/O error on kernel >= 5.11) #42177

Closed

giordano mentioned this pull request Jan 28, 2022

Internet access tests failing for UnprivilegedUserNamespacesExecutor when using Linux >= 5.11 JuliaContainerization/Sandbox.jl#77

Closed

Conversation

AkihiroSuda commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda commented Mar 10, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 16, 2021

Uh oh!

thaJeztah commented Mar 16, 2021

Uh oh!

thaJeztah commented Mar 16, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AkihiroSuda commented Feb 24, 2021 •

edited

Loading