Re: [PATCH 12/16] ovl: use vfs_{get,set}_fscaps() for copy-up

From: Amir Goldstein <amir73il@gmail.com>
To: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>,
	Serge Hallyn <serge@hallyn.com>,
	 Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>,
	 James Morris <jmorris@namei.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	 Miklos Szeredi <miklos@szeredi.hu>,
	linux-kernel@vger.kernel.org,  linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org,  audit@vger.kernel.org,
	linux-unionfs@vger.kernel.org
Subject: Re: [PATCH 12/16] ovl: use vfs_{get,set}_fscaps() for copy-up
Date: Thu, 30 Nov 2023 08:23:28 +0200	[thread overview]
Message-ID: <CAOQ4uxj=oR+yj19rUm0E6cHTiStniqvebtZSDhV3XZC1qz6n6A@mail.gmail.com> (raw)
In-Reply-To: <20231129-idmap-fscap-refactor-v1-12-da5a26058a5b@kernel.org>

On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean)
<sforshee@kernel.org> wrote:
>
> Using vfs_{get,set}xattr() for fscaps will be blocked in a future
> commit, so convert ovl to use the new interfaces. Also remove the now
> unused ovl_getxattr_value().
>
> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

You may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

I am assuming that this work is destined to be merged via the vfs tree?
Note that there is already a (non-conflicting) patch to copy_up.c on
Christian's vfs.rw branch.

I think it is best that all the overlayfs patches are tested together by
the vfs maintainer in preparation for the 6.8 merge window, so I have
a feeling that the 6.8 overlayfs PR is going to be merged via the vfs tree ;-)

Thanks,
Amir.

> ---
>  fs/overlayfs/copy_up.c | 72 ++++++++++++++++++++++++++------------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 4382881b0709..b43af5ce4b21 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -73,6 +73,23 @@ static int ovl_copy_acl(struct ovl_fs *ofs, const struct path *path,
>         return err;
>  }
>
> +static int ovl_copy_fscaps(struct ovl_fs *ofs, const struct path *oldpath,
> +                          struct dentry *new)
> +{
> +       struct vfs_caps capability;
> +       int err;
> +
> +       err = vfs_get_fscaps(mnt_idmap(oldpath->mnt), oldpath->dentry,
> +                            &capability);
> +       if (err) {
> +               if (err == -ENODATA || err == -EOPNOTSUPP)
> +                       return 0;
> +               return err;
> +       }
> +
> +       return vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), new, &capability, 0);
> +}
> +
>  int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct dentry *new)
>  {
>         struct dentry *old = oldpath->dentry;
> @@ -130,6 +147,14 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>                         break;
>                 }
>
> +               if (!strcmp(name, XATTR_NAME_CAPS)) {
> +                       error = ovl_copy_fscaps(OVL_FS(sb), oldpath, new);
> +                       if (!error)
> +                               continue;
> +                       /* fs capabilities must be copied */
> +                       break;
> +               }
> +
>  retry:
>                 size = ovl_do_getxattr(oldpath, name, value, value_size);
>                 if (size == -ERANGE)
> @@ -1006,61 +1031,40 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
>         return true;
>  }
>
> -static ssize_t ovl_getxattr_value(const struct path *path, char *name, char **value)
> -{
> -       ssize_t res;
> -       char *buf;
> -
> -       res = ovl_do_getxattr(path, name, NULL, 0);
> -       if (res == -ENODATA || res == -EOPNOTSUPP)
> -               res = 0;
> -
> -       if (res > 0) {
> -               buf = kzalloc(res, GFP_KERNEL);
> -               if (!buf)
> -                       return -ENOMEM;
> -
> -               res = ovl_do_getxattr(path, name, buf, res);
> -               if (res < 0)
> -                       kfree(buf);
> -               else
> -                       *value = buf;
> -       }
> -       return res;
> -}
> -
>  /* Copy up data of an inode which was copied up metadata only in the past. */
>  static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  {
>         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
>         struct path upperpath;
>         int err;
> -       char *capability = NULL;
> -       ssize_t cap_size;
> +       struct vfs_caps capability;
> +       bool has_capability = false;
>
>         ovl_path_upper(c->dentry, &upperpath);
>         if (WARN_ON(upperpath.dentry == NULL))
>                 return -EIO;
>
>         if (c->stat.size) {
> -               err = cap_size = ovl_getxattr_value(&upperpath, XATTR_NAME_CAPS,
> -                                                   &capability);
> -               if (cap_size < 0)
> +               err = vfs_get_fscaps(mnt_idmap(upperpath.mnt), upperpath.dentry,
> +                                    &capability);
> +               if (!err)
> +                       has_capability = 1;
> +               else if (err != -ENODATA && err != EOPNOTSUPP)
>                         goto out;
>         }
>
>         err = ovl_copy_up_data(c, &upperpath);
>         if (err)
> -               goto out_free;
> +               goto out;
>
>         /*
>          * Writing to upper file will clear security.capability xattr. We
>          * don't want that to happen for normal copy-up operation.
>          */
>         ovl_start_write(c->dentry);
> -       if (capability) {
> -               err = ovl_do_setxattr(ofs, upperpath.dentry, XATTR_NAME_CAPS,
> -                                     capability, cap_size, 0);
> +       if (has_capability) {
> +               err = vfs_set_fscaps(mnt_idmap(upperpath.mnt), upperpath.dentry,
> +                                    &capability, 0);
>         }
>         if (!err) {
>                 err = ovl_removexattr(ofs, upperpath.dentry,
> @@ -1068,13 +1072,11 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>         }
>         ovl_end_write(c->dentry);
>         if (err)
> -               goto out_free;
> +               goto out;
>
>         ovl_clear_flag(OVL_HAS_DIGEST, d_inode(c->dentry));
>         ovl_clear_flag(OVL_VERIFIED_DIGEST, d_inode(c->dentry));
>         ovl_set_upperdata(d_inode(c->dentry));
> -out_free:
> -       kfree(capability);
>  out:
>         return err;
>  }
>
> --
> 2.43.0
>