On 7/5/24 01:35, Amir Goldstein wrote>> ---
fs/overlayfs/super.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 06a231970cb5..4382f21c36a0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1394,6 +1394,19 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
if (IS_ERR(oe))
goto out_err;
+ if (ofs->numdatalayer) {
+ if (!ofs->config.metacopy) {
+ pr_err("lower data-only dirs require metacopy support.\n");
+ err = -EINVAL;
+ goto out_err;
+ }
Is that not already handled by?
int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
struct ovl_config *config)
{
struct ovl_opt_set set = ctx->set;
if (ctx->nr_data > 0 && !config->metacopy) {
pr_err("lower data-only dirs require metacopy support.\n");
return -EINVAL;
}
Probably because of:
ofs->config.metacopy = false;
pr_warn("...falling back to metacopy=off.\n");
Yes I think that's right.
in xattr check, but it could also happen from:
/* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
if (config->userxattr) {
...
/*
* Silently disable default setting of redirect and metacopy.
* This shall be the default in the future as well: these
* options must be explicitly enabled if used together with
* userxattr.
*/
config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
config->metacopy = false;
}
So maybe also the lowerdatadirs vs metacopy conflict should be moved
to the end of
ovl_fs_params_verify()?
Or possibly just remove it from ovl_fs_params_verify() completely and
only check it once, after all adjustments to metacopy have occurred?
FWIW, I'm personally more interested in seeing the data-only layers from
a user namespace case fail at mount time than these various other
scenarios where metacopy can get switched off. I'd also be happy to
confine a patch to detecting conflicts between no access to trusted.*
xattrs and any of the features that needs them.
+ if (!capable(CAP_SYS_ADMIN)) {
+ pr_err("lower data-only dirs require CAP_SYS_ADMIN in the initial user namespace.\n");
+ err = -EPERM;
+ goto out_err;
+ }
This is too specific IMO.
If we really want to check CAP_SYS_ADMIN at mount time, we should error
on any configuration that requires trusted xattrs and suggest that the user
will use -o userxattr, and maybe disable the conflicting config if it was not
explicitly specified in mount options.
Of course, userxattr conflicts with some other options including
redirect_dir, metacopy and verity, but that just means that the errors
will have to be smarter and the check for CAP_SYS_ADMIN should
definitely be in ovl_fs_params_verify() if we add them.
This makes a lot of sense to me. Thanks for your review & suggestions.
!capable(CAP_SYS_ADMIN) is the proxy I chose for "no access to trusted.*
xattrs" but I think it's a solid one as that's how xattr_permission()
does it. I was thinking calling xattr_permission() directly on the
first lowerdir or something to test if we can access trusted.* would
just complicate, but I could be wrong.
I'll take another stab at a patch in ovl_fs_params_verify() and focused
on no trusted.* access + metacopy/redirect/verity in coming days.
Thanks,
Mike
