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.
