daemon/graphdriver/overlay2: set TarOptions.InUserNS for native differ (fix "failed to Lchown "/dev/console") #48083
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since 838047a, the overlayWhiteoutConverter is supported with userns enabled, so we no longer need this check. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was discarding EPERM errors if it detected that userns was enabled; move such checks to the caller-site, so that they can decide how to handle the error (which, in case of userns may be to log and ignore). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
createTarFile accepts a opts (TarOptions) argument to specify whether userns is enabled; whe should consider always detecting locally, but at least make sure we're consistently working with the same value within this function. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commits b2fd67d (and the follow-up commit f6b8025) updated doesSupportNativeDiff to detect whether the host can support native overlay diffing with userns enabled. As a result, [useNaiveDiff] would now return "false" in cases where it previously would return "true" (and thus skip). However, [overlay2], unlike [fuse-overlay] did not take user namespaces into account, when using the native differ, and it therefore did not set the InUserNS option in TarOptions. As a result pkg/archive.createTarFile would attempt tocreate [device-nodes] through [handleTarTypeBlockCharFifo] which would fail, but the resulting error `EPERM` would be discarded, and `createTarFile` would not return early, therefor attempting to [os.LChown] the missing file, ultimately resulting in an error: failed to Lchown "/dev/console" for UID 0, GID 0: lchown /dev/console: no such file or directory This patch fixes the missing option in overlay. [useNaiveDiff]: https://github.com/moby/moby/blob/47eebd718f332f29a38455b61ee879ced5bc219b/daemon/graphdriver/overlay2/overlay.go#L248-L256 [overlay2]: https://github.com/moby/moby/blob/47eebd718f332f29a38455b61ee879ced5bc219b/daemon/graphdriver/overlay2/overlay.go#L684-L689 [fuse-overlay]: https://github.com/moby/moby/blob/47eebd718f332f29a38455b61ee879ced5bc219b/daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go#L456-L462 [device-nodes]: https://github.com/moby/moby/blob/ff1e2c0de72a1bbbe4cdbe1558da57d327899df5/pkg/archive/archive.go#L713-L720 [handleTarTypeBlockCharFifo]: https://github.com/moby/moby/blob/47eebd718f332f29a38455b61ee879ced5bc219b/pkg/archive/archive_unix.go#L110-L114 [os.LChown]: https://github.com/moby/moby/blob/ff1e2c0de72a1bbbe4cdbe1558da57d327899df5/pkg/archive/archive.go#L762-L773 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
dmcgowan
reviewed
Jun 28, 2024
| // Handle this is an OS-specific way | ||
| if err := handleTarTypeBlockCharFifo(hdr, path); err != nil { | ||
| if inUserns && errors.Is(err, syscall.EPERM) { | ||
| // In most cases, cannot create a fifo if running in user namespace |
Member
There was a problem hiding this comment.
Beside the previous code stumbled into this, is Fifo really something we don't expect to be working. In containerd we don't skip Fifo for user namespaces.
Member
Author
There was a problem hiding this comment.
I went a bit back-and-forth on both of these;
- changed all of
TypeFifo,TypeBlock, andTypeCharto try optimistically (but log the error on failure); this in the line of thought "who knows? one day it may work?" - or the reverse; anticipating it won't work, so skip for
useNs
In the end, I decided to keep the current behavior for now, but perhaps that's something we should revisit, WDYT?
Member
There was a problem hiding this comment.
I think keep current behavior is right for the scope of this. Just the behavior difference was sticking out more so wanted to mention it.
Member
Author
There was a problem hiding this comment.
Yup, same! It also was a bit my motivation for moving the error-handling out of handleTarTypeBlockCharFifo - I wanted to make it more visible / explicit where we're ignoring errors.
dmcgowan
approved these changes
Jun 28, 2024
corhere
approved these changes
Jun 28, 2024
Member
Author
|
I'll bring this one in, and move the backport out of draft. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pkg/archive: getWhiteoutConverter: don't error with userns enabled
Since 838047a, the overlayWhiteoutConverter
is supported with userns enabled, so we no longer need this check.
pkg/archive: handleTarTypeBlockCharFifo: don't discard EPERM errors
This function was discarding EPERM errors if it detected that userns was
enabled; move such checks to the caller-site, so that they can decide
how to handle the error (which, in case of userns may be to log and ignore).
pkg/archive: createTarFile: consistently use the same value for userns
createTarFile accepts a opts (TarOptions) argument to specify whether
userns is enabled; whe should consider always detecting locally, but
at least make sure we're consistently working with the same value within
this function.
daemon/graphdriver/overlay2: set TarOptions.InUserNS for native differ
Commits b2fd67d (and the follow-up commit
f6b8025) updated doesSupportNativeDiff to
detect whether the host can support native overlay diffing with userns
enabled.
As a result, useNaiveDiff would now return "false" in cases where it
previously would return "true" (and thus skip). However, overlay2,
unlike fuse-overlay did not take user namespaces into account, when
using the native differ, and it therefore did not set the InUserNS option
in TarOptions.
As a result pkg/archive.createTarFile would attempt tocreate device-nodes
through handleTarTypeBlockCharFifo which would fail, but the resulting
error
EPERMwould be discarded, andcreateTarFilewould not returnearly, therefor attempting to os.LChown the missing file, ultimately
resulting in an error:
This patch fixes the missing option in overlay.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)