Re: [PATCH v2 20/25] ovl: add fscaps handlers

["Seth Forshee (DigitalOcean)" <sforshee@kernel.org>]

On Tue, Feb 27, 2024 at 03:28:18PM +0200, Amir Goldstein wrote:
> > +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 = 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;
> > +
> > +       /*
> > +        * If the fscaps are to be remove from a lower file, check that they
> > +        * exist before copying up.
> > +        */
> 
> Don't you need to convert -ENODATA to 0 return value in this case?

Do you mean that trying to remove an xattr that does not exist should
return 0? Standard behavior is to return -ENODATA in this situation.

> 
> > +       if (!caps && !upperdentry) {
> > +               struct path realpath;
> > +               struct vfs_caps lower_caps;
> > +
> > +               ovl_path_lower(dentry, &realpath);
> > +               old_cred = ovl_override_creds(dentry->d_sb);
> > +               err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realdentry,
> > +                                    &lower_caps);
> > +               revert_creds(old_cred);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +       err = 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()")

Fixed.

> > @@ -758,6 +826,8 @@ static const struct inode_operations ovl_symlink_inode_operations = {
> >         .get_link       = ovl_get_link,
> >         .getattr        = ovl_getattr,
> >         .listxattr      = ovl_listxattr,
> > +       .get_fscaps     = ovl_get_fscaps,
> > +       .set_fscaps     = ovl_set_fscaps,
> >         .update_time    = ovl_update_time,
> >  };
> >
> > @@ -769,6 +839,8 @@ static const struct inode_operations ovl_special_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,
> >         .update_time    = 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.

The kernel does not forbid XATTR_NAME_CAPS for non-regular files and
will internally even try to read them from non-regular files during
killpriv checks. If we do not add handlers then we will end up using the
normal ovl xattr handlers, which call vfs_*xattr(). These will return an
error for fscaps xattrs after this series, which would be a change in
behavior for overlayfs and make it behave differently from other
filesystems.