Prefer user.containers.override_stat over user.fuseoverlayfs. #422
Conversation
This file contains 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
|
I'll merge once CI is green. Thanks for working on this! I am wondering at this point, if it would be easier to always use |
|
does this PR need a rebase? |
akihikodaki
force-pushed
the
containers
branch
2 times, most recently
from
c772c22
to
2a815a4
Compare
Signed-off-by: Akihiko Odaki <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
override_mode () used to suppress ENODATA only in a certain condition. ENODATA errors in other situations made load_dir () fail because it indirectly calls override_mode () when the underlying file system reports DT_UNKNOWN for an opaque whiteout file and such an file does not have mode xattrs. do_fchmod () and do_chmod () worked around the problem by supressing ENODATA by themselves, but that led to code duplication. Always suppress ENODATA to resolve these problems. Signed-off-by: Akihiko Odaki <[email protected]>
do_fchmod () and do_chmod () used to call override_mode () directly to retrieve the owner information, but the usage of override_mode () was wrong; override_mode () expects struct stat is already populated by the information provided by the underlying filesystem, but do_fchmod () and do_chmod () only zeroed st_uid and st_gid. override_mode () does not update the owner information when st_mode is not S_IFDIR nor S_IFREG so this caused chmod to change the file owner to root at random. Use the logic rpl_stat () employs to file owner retrieval for chmod functions to ensure they provide the owner information consistent with rpl_stat (). Signed-off-by: Akihiko Odaki <[email protected]>
akihikodaki
force-pushed
the
containers
branch
4 times, most recently
from
5cec1d1
to
8f9b942
Compare
ovl_setattr () used to pass -1 as uid or gid when either of them is not changed for do_fchown () / do_chown (), but if these functions use overriding xattrs instead of real fchown () or chown (), it causes -1 to be written in owner overriding xattrs and break them. Replace -1 with the current uid or gid before calling do_fchown () / do_chown (). Signed-off-by: Akihiko Odaki <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
Previously, fuse-overlayfs always used user.fuseoverlayfs.override_stat for the upper layer while honoring user.containers.override_stat for lower layers so that it can consume a layer created by containers/storage. It turned out that containers/storage also needs to get the overriding extended attribute set by fuse-overlayfs and to set one for the upper layer to make the root directory of the upper layer inherit the mode of a lower layer. Adding code to get and to set user.fuseoverlayfs.override_stat to containers/storage is a bit ugly. The underlying problem is that fuse-overlayfs changes what name to use ad hoc. Fix it by always preferring user.containers.override_stat, which containers/storage honors, over user.fuseoverlayfs.overlayfs, which is specific to fuse-overlayfs. Signed-off-by: Akihiko Odaki <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
The major use case of stat override is to enable rootless containers on network filesystems, and they also lack security xattr support in non-root user namespaces. Trying to set security xattrs on them result in ENOTSUP and break things. It makes little sense to share security xattrs with the underlying filesystems when overriding stat in the first place. Linux's NFS server exposes security xattrs only when the user explicitly claims the security consistencies between the server and clients, and hide them otherwise. Following this precedent, we should isolate security xattrs since we know the security policy enforced by fuse-overlayfs is already distinct from the underlying filesystem when overriding owners and file mode. Mark security xattrs inaccessible with STAT_OVERRIDE_CONTAINERS to prefix all access to them with XATTR_CONTAINERS_OVERRIDE_PREFIX. Signed-off-by: Akihiko Odaki <[email protected]>
|
Rebased and fixed tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
See containers/storage#1953 (comment)