Thread (45 messages) 45 messages, 5 authors, 2023-12-11

Re: [PATCH 11/16] ovl: add fscaps handlers

From: Amir Goldstein <amir73il@gmail.com>
Date: 2023-11-30 05:57:01
Also in: linux-fsdevel, linux-unionfs, lkml

On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean)
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
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_operations = {
        .get_inode_acl  = ovl_get_inode_acl,
        .get_acl        = ovl_get_acl,
        .set_acl        = ovl_set_acl,
+       .get_fscaps     = ovl_get_fscaps,
+       .set_fscaps     = ovl_set_fscaps,
+       .remove_fscaps  = ovl_remove_fscaps,
        .update_time    = ovl_update_time,
        .fileattr_get   = ovl_fileattr_get,
        .fileattr_set   = 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 dentry *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 = ovl_override_creds(dentry->d_sb);
+       err = 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 = OVL_FS(dentry->d_sb);
+       struct dentry *upperdentry = ovl_dentry_upper(dentry);
+       struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
+       const struct cred *old_cred;
+
+       err = ovl_want_write(dentry);
+       if (err)
+               goto out;
+
+       if (!upperdentry) {
+               err = ovl_copy_up(dentry);
+               if (err)
+                       goto out_drop_write;
+
+               realdentry = ovl_dentry_upper(dentry);
+       }
+
+       old_cred = ovl_override_creds(dentry->d_sb);
+       err = 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 = OVL_FS(dentry->d_sb);
+       struct dentry *upperdentry = ovl_dentry_upper(dentry);
+       struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
+       const struct cred *old_cred;
+
+       err = ovl_want_write(dentry);
+       if (err)
+               goto out;
+
+       if (!upperdentry) {
+               err = ovl_copy_up(dentry);
+               if (err)
+                       goto out_drop_write;
+
+               realdentry = ovl_dentry_upper(dentry);
+       }
+
This construct is peculiar.
Most of the operations just do this unconditionally:

err = 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 = 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help