Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231129-idmap-fscap-refactor-v1-0-da5a26058a5b@kernel.org> <20231129-idmap-fscap-refactor-v1-11-da5a26058a5b@kernel.org>
In-Reply-To: <20231129-idmap-fscap-refactor-v1-11-da5a26058a5b@kernel.org>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 30 Nov 2023 07:56:48 +0200
Message-ID: <CAOQ4uxiuFmvaA6yd59WMGxWAGKc6JBono3oNp4XndYcfWVhUxw@mail.gmail.com>
Subject: Re: [PATCH 11/16] ovl: add fscaps handlers
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 29, 2023 at 11:50=E2=80=AFPM Seth Forshee (DigitalOcean)
<sforshee@kernel.org> wrote:
>
> Add handlers which read fs caps from the lower or upper filesystem and
> write/remove fs caps to the upper filesystem, performing copy-up as
> necessary.
>
> While it doesn't make sense to use fscaps on directories, nothing in the
> kernel actually prevents setting or getting fscaps xattrs for directory
> inodes. If we omit fscaps handlers in ovl_dir_inode_operations then the
> generic handlers will be used. These handlers will use the xattr inode
> operations, bypassing any idmapping on lower mounts, so fscaps handlers
> are also installed for ovl_dir_inode_operations.
>
> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
> ---
>  fs/overlayfs/dir.c       |  3 ++
>  fs/overlayfs/inode.c     | 84 ++++++++++++++++++++++++++++++++++++++++++=
++++++
>  fs/overlayfs/overlayfs.h |  6 ++++
>  3 files changed, 93 insertions(+)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index aab3f5d93556..d9ab3c9ce10a 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1303,6 +1303,9 @@ const struct inode_operations ovl_dir_inode_operati=
ons =3D {
>         .get_inode_acl  =3D ovl_get_inode_acl,
>         .get_acl        =3D ovl_get_acl,
>         .set_acl        =3D ovl_set_acl,
> +       .get_fscaps     =3D ovl_get_fscaps,
> +       .set_fscaps     =3D ovl_set_fscaps,
> +       .remove_fscaps  =3D ovl_remove_fscaps,
>         .update_time    =3D ovl_update_time,
>         .fileattr_get   =3D ovl_fileattr_get,
>         .fileattr_set   =3D ovl_fileattr_set,
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index c63b31a460be..82fc6e479d45 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -568,6 +568,87 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dent=
ry *dentry,
>  }
>  #endif
>
> +int ovl_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> +                  struct vfs_caps *caps)
> +{
> +       int err;
> +       const struct cred *old_cred;
> +       struct path realpath;
> +
> +       ovl_path_real(dentry, &realpath);
> +       old_cred =3D ovl_override_creds(dentry->d_sb);
> +       err =3D vfs_get_fscaps(mnt_idmap(realpath.mnt), realpath.dentry, =
caps);
> +       revert_creds(old_cred);
> +       return err;
> +}
> +
> +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> +                  const struct vfs_caps *caps, int flags)
> +{
> +       int err;
> +       struct ovl_fs *ofs =3D OVL_FS(dentry->d_sb);
> +       struct dentry *upperdentry =3D ovl_dentry_upper(dentry);
> +       struct dentry *realdentry =3D upperdentry ?: ovl_dentry_lower(den=
try);
> +       const struct cred *old_cred;
> +
> +       err =3D ovl_want_write(dentry);
> +       if (err)
> +               goto out;
> +
> +       if (!upperdentry) {
> +               err =3D ovl_copy_up(dentry);
> +               if (err)
> +                       goto out_drop_write;
> +
> +               realdentry =3D ovl_dentry_upper(dentry);
> +       }
> +
> +       old_cred =3D ovl_override_creds(dentry->d_sb);
> +       err =3D vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), realdentry, caps=
, flags);
> +       revert_creds(old_cred);
> +
> +       /* copy c/mtime */
> +       ovl_copyattr(d_inode(dentry));
> +
> +out_drop_write:
> +       ovl_drop_write(dentry);
> +out:
> +       return err;
> +}
> +
> +int ovl_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry)
> +{
> +       int err;
> +       struct ovl_fs *ofs =3D OVL_FS(dentry->d_sb);
> +       struct dentry *upperdentry =3D ovl_dentry_upper(dentry);
> +       struct dentry *realdentry =3D upperdentry ?: ovl_dentry_lower(den=
try);
> +       const struct cred *old_cred;
> +
> +       err =3D ovl_want_write(dentry);
> +       if (err)
> +               goto out;
> +
> +       if (!upperdentry) {
> +               err =3D ovl_copy_up(dentry);
> +               if (err)
> +                       goto out_drop_write;
> +
> +               realdentry =3D ovl_dentry_upper(dentry);
> +       }
> +

This construct is peculiar.
Most of the operations just do this unconditionally:

err =3D ovl_copy_up(dentry);
if (err)
        goto out_drop_write;

and then use ovl_dentry_upper(dentry) directly, because a modification
will always be done on the upper dentry, regardless of the state before
the operation started.

I was wondering where you copied this from and I found it right above
in ovl_set_or_remove_acl().
In that case, there was also no justification for this construct.

There is also no justification for open coding:
   struct dentry *realdentry =3D upperdentry ?: ovl_dentry_lower(dentry);
when later on, ovl_path_lower(dentry, &realpath) is called anyway.

The only reason to do anything special in ovl_set_or_remove_acl() is:

        /*
         * If ACL is to be removed from a lower file, check if it exists in
         * the first place before copying it up.
         */

Do you not want to do the same for ovl_remove_fscaps()?

Also, the comparison to remove_acl API bares the question,
why did you need to add a separate method for remove_fscaps?
Why not use set_fscaps(NULL), just like setxattr() and set_acl() APIs?

Thanks,
Amir.