Received-SPF: pass (google.com: domain of linux-kernel+bounces-83315-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240221-idmap-fscap-refactor-v2-0-3039364623bd@kernel.org> <20240221-idmap-fscap-refactor-v2-20-3039364623bd@kernel.org>
In-Reply-To: <20240221-idmap-fscap-refactor-v2-20-3039364623bd@kernel.org>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 27 Feb 2024 15:28:18 +0200
Message-ID: <CAOQ4uxjvrFuz2iCiO9dsOnear+qN=M+GFW-eEOZU5uCzBkTwLQ@mail.gmail.com>
Subject: Re: [PATCH v2 20/25] 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>, Jan Kara <jack@suse.cz>, 
	Stephen Smalley <stephen.smalley.work@gmail.com>, Ondrej Mosnacek <omosnace@redhat.com>, 
	Casey Schaufler <casey@schaufler-ca.com>, Mimi Zohar <zohar@linux.ibm.com>, 
	Roberto Sassu <roberto.sassu@huawei.com>, Dmitry Kasatkin <dmitry.kasatkin@gmail.com>, 
	Eric Snowberg <eric.snowberg@oracle.com>, "Matthew Wilcox (Oracle)" <willy@infradead.org>, 
	Jonathan Corbet <corbet@lwn.net>, 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, selinux@vger.kernel.org, 
	linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org, 
	linux-unionfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 21, 2024 at 11:25=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 fscaps only really make sense on regular files, the general policy
> is to allow most xattr namespaces on all different inode types, so
> fscaps handlers are installed in the inode operations for all types of
> inodes.
>
> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
> ---
>  fs/overlayfs/dir.c       |  2 ++
>  fs/overlayfs/inode.c     | 72 ++++++++++++++++++++++++++++++++++++++++++=
++++++
>  fs/overlayfs/overlayfs.h |  5 ++++
>  3 files changed, 79 insertions(+)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 0f8b4a719237..4ff360fe10c9 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1307,6 +1307,8 @@ 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,
>         .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..7a8978ea6fe1 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -568,6 +568,72 @@ 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 setxattr_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;
> +
> +       /*
> +        * If the fscaps are to be remove from a lower file, check that t=
hey
> +        * exist before copying up.
> +        */

Don't you need to convert -ENODATA to 0 return value in this case?

> +       if (!caps && !upperdentry) {
> +               struct path realpath;
> +               struct vfs_caps lower_caps;
> +
> +               ovl_path_lower(dentry, &realpath);
> +               old_cred =3D ovl_override_creds(dentry->d_sb);
> +               err =3D vfs_get_fscaps(mnt_idmap(realpath.mnt), realdentr=
y,
> +                                    &lower_caps);
> +               revert_creds(old_cred);
> +               if (err)
> +                       goto out;
> +       }
> +
> +       err =3D ovl_want_write(dentry);
> +       if (err)
> +               goto out;
> +

ovl_want_write() should after ovl_copy_up(), see:
162d06444070 ("ovl: reorder ovl_want_write() after ovl_inode_lock()")


> +       err =3D ovl_copy_up(dentry);
> +       if (err)
> +               goto out_drop_write;
> +       upperdentry =3D ovl_dentry_upper(dentry);
> +
> +       old_cred =3D ovl_override_creds(dentry->d_sb);
> +       if (!caps)
> +               err =3D vfs_remove_fscaps(ovl_upper_mnt_idmap(ofs), upper=
dentry);
> +       else
> +               err =3D vfs_set_fscaps(ovl_upper_mnt_idmap(ofs), upperden=
try,
> +                                    caps, setxattr_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_update_time(struct inode *inode, int flags)
>  {
>         if (flags & S_ATIME) {
> @@ -747,6 +813,8 @@ static const struct inode_operations ovl_file_inode_o=
perations =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,
>         .update_time    =3D ovl_update_time,
>         .fiemap         =3D ovl_fiemap,
>         .fileattr_get   =3D ovl_fileattr_get,
> @@ -758,6 +826,8 @@ static const struct inode_operations ovl_symlink_inod=
e_operations =3D {
>         .get_link       =3D ovl_get_link,
>         .getattr        =3D ovl_getattr,
>         .listxattr      =3D ovl_listxattr,
> +       .get_fscaps     =3D ovl_get_fscaps,
> +       .set_fscaps     =3D ovl_set_fscaps,
>         .update_time    =3D ovl_update_time,
>  };
>
> @@ -769,6 +839,8 @@ static const struct inode_operations ovl_special_inod=
e_operations =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,
>         .update_time    =3D ovl_update_time,
>  };
>


Sorry, I did not understand the explanation why fscaps ops are needed
for non regular files. It does not look right to me.

Thanks,
Amir.