On Thu, Nov 30, 2023 at 07:56:48AM +0200, Amir Goldstein wrote:
> On Wed, Nov 29, 2023 at 11:50 PM Seth Forshee (DigitalOcean)
> <sforshee@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
Yes, I did use ovl_set_or_remove_acl() as a reference here. But looking
around at some of the other code, I see what you mean, so I'll rework
this code.
> 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()?
Yes, I will add this.
> 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?
I had started on these patches a while back and then picked them up
again recently, and honestly I don't remember why I did that originally.
I don't see a need for it now though, so I can change that.
