Re: [PATCH 4/5] ovl: don't require metacopy=on for lower -> data redirect

[Amir Goldstein <amir73il@gmail.com>]

> Subject: [PATCH] ovl: don't require metacopy=on for lower -> data redirect

Nit: the change does not require metacopy=on nor redirect_dir=follow
so maybe some short title like:

ovl: relax redirect/metacopy requirements for lower -> data redirect


> Allow the special case of a redirect from a lower layer to a data layer
> without having to turn on metacopy.  This makes the feature work with
> userxattr, which in turn allows data layers to be usable in user
> namespaces.
>
> Minimize the risk by only enabling redirect from a single lower layer to a
> data layer iff a data layer is specified.  The only way to access a data
> layer is to enable this, so there's really no reason no to enable this.
>
> This can be used safely if the lower layer is read-only and the
> user.overlay.redirect xattr cannot be modified.
>

Just need to verify that those assumptions are not broken by
upper index with metacopy/redirect -
I think the safety of redirect still holds, but not the safety
of the verity digest.

Thanks,
Amir.

> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst |  7 ++++++
>  fs/overlayfs/namei.c                    | 32 ++++++++++++++-----------
>  fs/overlayfs/params.c                   |  5 ----
>  3 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 6245b67ae9e0..5d277d79cf2f 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -429,6 +429,13 @@ Only the data of the files in the "data-only" lower layers may be visible
>  when a "metacopy" file in one of the lower layers above it, has a "redirect"
>  to the absolute path of the "lower data" file in the "data-only" lower layer.
>
> +Instead of explicitly enabling "metacopy=on" it is sufficient to specify at
> +least one data-only layer to enable redirection of data to a data-only layer.
> +In this case other forms of metacopy are rejected.  Note: this way data-only
> +layers may be used toghether with "userxattr", in which case careful attention
> +must be given to privileges needed to change the "user.overlay.redirect" xattr
> +to prevent misuse.
> +
>  Since kernel version v6.8, "data-only" lower layers can also be added using
>  the "datadir+" mount options and the fsconfig syscall from new mount api.
>  For example::
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index da322e9768d1..f9dc71b70beb 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1042,6 +1042,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         char *upperredirect = NULL;
>         bool nextredirect = false;
>         bool nextmetacopy = false;
> +       bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
>         struct dentry *this;
>         unsigned int i;
>         int err;
> @@ -1053,7 +1054,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .is_dir = false,
>                 .opaque = false,
>                 .stop = false,
> -               .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> +               .last = check_redirect ? false : !ovl_numlower(poe),
>                 .redirect = NULL,
>                 .metacopy = 0,
>         };
> @@ -1141,7 +1142,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_put;
>                 }
>
> -               if (!ovl_redirect_follow(ofs))
> +               if (!check_redirect)
>                         d.last = i == ovl_numlower(poe) - 1;
>                 else if (d.is_dir || !ofs->numdatalayer)
>                         d.last = lower.layer->idx == ovl_numlower(roe);
> @@ -1222,21 +1223,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>         }
>
> -       /* Defer lookup of lowerdata in data-only layers to first access */
> +       /*
> +        * Defer lookup of lowerdata in data-only layers to first access.
> +        * Don't require redirect=follow and metacopy=on in this case.
> +        */
>         if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
>                 d.metacopy = 0;
>                 ctr++;
> -       }
> -
> -       if (nextmetacopy && !ofs->config.metacopy) {
> -               pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> -               err = -EPERM;
> -               goto out_put;
> -       }
> -       if (nextredirect && !ovl_redirect_follow(ofs)) {
> -               pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> -               err = -EPERM;
> -               goto out_put;
> +       } else {
> +               if (nextmetacopy && !ofs->config.metacopy) {
> +                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
> +               if (nextredirect && !ovl_redirect_follow(ofs)) {
> +                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
>         }
>
>         /*
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 1115c22deca0..54468b2b0fba 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -1000,11 +1000,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                  */
>         }
>
> -       if (ctx->nr_data > 0 && !config->metacopy) {
> -               pr_err("lower data-only dirs require metacopy support.\n");
> -               return -EINVAL;
> -       }
> -
>         return 0;
>  }
>
> --
> 2.48.1
>