| Message ID | 20231129-idmap-fscap-refactor-v1-16-da5a26058a5b@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
On Wed, Nov 29, 2023 at 11:51 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > > Now that the new vfs-level interfaces are fully supported and all code > has been converted to use them, stop permitting use of the top-level vfs > xattr interfaces for capabilities xattrs. Unlike with ACLs we still need > to be able to work with fscaps xattrs using lower-level interfaces in a > handful of places, so only use of the top-level xattr interfaces is > restricted. Can you explain why? Is there an inherent difference between ACLs and fscaps in that respect or is it just a matter of more work that needs to be done? > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > --- > fs/xattr.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/xattr.c b/fs/xattr.c > index 372644b15457..4b779779ad8c 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -540,6 +540,9 @@ vfs_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, > const void *orig_value = value; > int error; > > + if (!strcmp(name, XATTR_NAME_CAPS)) > + return -EOPNOTSUPP; > + It this is really not expected, then it should be an assert and please use an inline helper like is_posix_acl_xattr(): if (WARN_ON_ONCE(is_fscaps_xattr(name))) It wouldn't hurt to add those assertions to is_posix_acl_xattr() cases as well, but that is unrelated to your change. Thanks, Amir.
On Thu, Nov 30, 2023 at 08:10:15AM +0200, Amir Goldstein wrote: > On Wed, Nov 29, 2023 at 11:51 PM Seth Forshee (DigitalOcean) > <sforshee@kernel.org> wrote: > > > > Now that the new vfs-level interfaces are fully supported and all code > > has been converted to use them, stop permitting use of the top-level vfs > > xattr interfaces for capabilities xattrs. Unlike with ACLs we still need > > to be able to work with fscaps xattrs using lower-level interfaces in a > > handful of places, so only use of the top-level xattr interfaces is > > restricted. > > Can you explain why? > Is there an inherent difference between ACLs and fscaps in that respect > or is it just a matter of more work that needs to be done? There are a number of differences. ACLs have caching, require additional permission checks, and require a lot of filesystem-specific handling. fscaps are simpler by comparison, and most filesystems can rely on a common implementation that just converts to/from raw disk xattrs. So at minimum I think the lowest level interfaces, __vfs_{get,set,remove}xattr(), need to continue to allow fscaps, and that's where ACL xattrs are blocked. Allowing some of the others to still work with them is a matter of convenience (e.g. using vfs_getxattr_alloc()) and trying to reduce code duplication. But as you pointed out I did miss at least duplicating fsnotify_xattr(), so I'm going to have another look at how I implemented these. > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > --- > > fs/xattr.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/fs/xattr.c b/fs/xattr.c > > index 372644b15457..4b779779ad8c 100644 > > --- a/fs/xattr.c > > +++ b/fs/xattr.c > > @@ -540,6 +540,9 @@ vfs_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, > > const void *orig_value = value; > > int error; > > > > + if (!strcmp(name, XATTR_NAME_CAPS)) > > + return -EOPNOTSUPP; > > + > > It this is really not expected, then it should be an assert and > please use an inline helper like is_posix_acl_xattr(): > > if (WARN_ON_ONCE(is_fscaps_xattr(name))) Ack, makes sense.
diff --git a/fs/xattr.c b/fs/xattr.c index 372644b15457..4b779779ad8c 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -540,6 +540,9 @@ vfs_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, const void *orig_value = value; int error; + if (!strcmp(name, XATTR_NAME_CAPS)) + return -EOPNOTSUPP; + retry_deleg: inode_lock(inode); error = __vfs_setxattr_locked(idmap, dentry, name, value, size, @@ -655,6 +658,9 @@ vfs_getxattr(struct mnt_idmap *idmap, struct dentry *dentry, struct inode *inode = dentry->d_inode; int error; + if (!strcmp(name, XATTR_NAME_CAPS)) + return -EOPNOTSUPP; + error = xattr_permission(idmap, inode, name, MAY_READ); if (error) return error; @@ -794,6 +800,9 @@ vfs_removexattr(struct mnt_idmap *idmap, struct dentry *dentry, struct inode *delegated_inode = NULL; int error; + if (!strcmp(name, XATTR_NAME_CAPS)) + return -EOPNOTSUPP; + retry_deleg: inode_lock(inode); error = __vfs_removexattr_locked(idmap, dentry,
Now that the new vfs-level interfaces are fully supported and all code has been converted to use them, stop permitting use of the top-level vfs xattr interfaces for capabilities xattrs. Unlike with ACLs we still need to be able to work with fscaps xattrs using lower-level interfaces in a handful of places, so only use of the top-level xattr interfaces is restricted. Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> --- fs/xattr.c | 9 +++++++++ 1 file changed, 9 insertions(+)