| Message ID | 20230125-fs-acl-remove-generic-xattr-handlers-v2-1-214cfb88bb56@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acl: remove generic posix acl handlers from all xattr handlers | expand |
On Mon, Jan 30, 2023 at 05:41:57PM +0100, Christian Brauner wrote: > The POSIX ACL api doesn't use the xattr handler infrastructure anymore. > If we keep relying on IOP_XATTR we will have to find a way to raise > IOP_XATTR during inode_init_always() if a filesystem doesn't implement > any xattrs other than POSIX ACLs. That's not done today but is in > principle possible. A prior version introduced SB_I_XATTR to this end. > Instead of this affecting all filesystems let those filesystems that > explicitly disable xattrs for some inodes disable POSIX ACLs by raising > IOP_NOACL. I'm still a little confused about this, and also about inode_xattr_disable. More below: > - if (!(old->d_inode->i_opflags & IOP_XATTR) || > - !(new->d_inode->i_opflags & IOP_XATTR)) > + if (inode_xattr_disabled(old->d_inode) || > + inode_xattr_disabled(new->d_inode)) This code shouldn't care about ACLs because the copy up itself should be all based around the ACL API, no? > + if (!(inode->i_opflags & IOP_NOACL)) > error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); > else if (unlikely(is_bad_inode(inode))) > error = -EIO; > @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry, > if (error) > goto out_inode_unlock; > > - if (inode->i_opflags & IOP_XATTR) > + if (!(inode->i_opflags & IOP_NOACL)) > error = set_posix_acl(mnt_userns, dentry, acl_type, NULL); And here the lack of get/set methods should be all we need unless I'm missing something? > diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c > index c7d1fa526dea..2a7037b165f0 100644 > --- a/fs/reiserfs/inode.c > +++ b/fs/reiserfs/inode.c > @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, > */ > if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { > inode->i_flags |= S_PRIVATE; > - inode->i_opflags &= ~IOP_XATTR; > + inode_xattr_disable(inode); I'll need to hear from the reiserfs maintainers, but this also seems like something that would be better done based on method presence. > diff --git a/fs/xattr.c b/fs/xattr.c > index adab9a70b536..89b6c122056d 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > error = security_inode_listxattr(dentry); > if (error) > return error; > - if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) { > + if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) { > error = inode->i_op->listxattr(dentry, list, size); So once listing ACLs is moved out of ->listxattr there should be no need to check anything for ACLs here either.
On Mon, Jan 30, 2023 at 05:50:53PM +0100, Christoph Hellwig wrote: > On Mon, Jan 30, 2023 at 05:41:57PM +0100, Christian Brauner wrote: > > The POSIX ACL api doesn't use the xattr handler infrastructure anymore. > > If we keep relying on IOP_XATTR we will have to find a way to raise > > IOP_XATTR during inode_init_always() if a filesystem doesn't implement > > any xattrs other than POSIX ACLs. That's not done today but is in > > principle possible. A prior version introduced SB_I_XATTR to this end. > > Instead of this affecting all filesystems let those filesystems that > > explicitly disable xattrs for some inodes disable POSIX ACLs by raising > > IOP_NOACL. > > I'm still a little confused about this, and also about > inode_xattr_disable. More below: > > > - if (!(old->d_inode->i_opflags & IOP_XATTR) || > > - !(new->d_inode->i_opflags & IOP_XATTR)) > > + if (inode_xattr_disabled(old->d_inode) || > > + inode_xattr_disabled(new->d_inode)) > > This code shouldn't care about ACLs because the copy up itself > should be all based around the ACL API, no? The loop copies up all xattrs. It retrieves all xattrs via vfs_listxattr() then walks through all of them and copies them up. But it's nothing that we couldn't work around if it buys as less headaches overall. > > > + if (!(inode->i_opflags & IOP_NOACL)) > > error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); > > else if (unlikely(is_bad_inode(inode))) > > error = -EIO; > > @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry, > > if (error) > > goto out_inode_unlock; > > > > - if (inode->i_opflags & IOP_XATTR) > > + if (!(inode->i_opflags & IOP_NOACL)) > > error = set_posix_acl(mnt_userns, dentry, acl_type, NULL); > > And here the lack of get/set methods should be all we need unless > I'm missing something? For setting acl that should work, yes. > > > diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c > > index c7d1fa526dea..2a7037b165f0 100644 > > --- a/fs/reiserfs/inode.c > > +++ b/fs/reiserfs/inode.c > > @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, > > */ > > if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { > > inode->i_flags |= S_PRIVATE; > > - inode->i_opflags &= ~IOP_XATTR; > > + inode_xattr_disable(inode); > > I'll need to hear from the reiserfs maintainers, but this also seems > like something that would be better done based on method presence. I mean, since this is locked I would think we could just: inode->i_op->{g,s}et_acl = NULL and for btrfs it should work to as it uses simple_dir_inode_operations which doesn't implement get/set posix acl methods. > > > diff --git a/fs/xattr.c b/fs/xattr.c > > index adab9a70b536..89b6c122056d 100644 > > --- a/fs/xattr.c > > +++ b/fs/xattr.c > > @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > > error = security_inode_listxattr(dentry); > > if (error) > > return error; > > - if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) { > > + if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) { > > error = inode->i_op->listxattr(dentry, list, size); > > So once listing ACLs is moved out of ->listxattr there should be no > need to check anything for ACLs here either. I think so... But that would mean we'd need to change the ->listxattr() inode operation to not return POSIX ACLs anymore. Instead vfs_listxattr() would issue two vfs_get_acl() calls to check whether POSIX ACLs are supported and if so append them to the buffer. That seems doable as vfs_listxattr() is rarely used in the fs/ and nowhere in security/ luckily. Wdyt? The only thing potentially wrong with that is that's two more filesystem calls which probably doesn't matter for listing xattrs as that isn't really that fast anyway and the ->listxattr() api is broken beyond repair for userspace anyway. I would need to check tomorrow whether we run into any real issues with this idea but it could work. If we're lucky it might mean that we could get rid of the generic POSIX ACL handler dependency even for ext2/ext4/erofs/f2fs/reiserfs/jffs2/ocfs2.
On Mon, Jan 30, 2023 at 07:09:09PM +0100, Christian Brauner wrote: > On Mon, Jan 30, 2023 at 05:50:53PM +0100, Christoph Hellwig wrote: > > On Mon, Jan 30, 2023 at 05:41:57PM +0100, Christian Brauner wrote: > > > The POSIX ACL api doesn't use the xattr handler infrastructure anymore. > > > If we keep relying on IOP_XATTR we will have to find a way to raise > > > IOP_XATTR during inode_init_always() if a filesystem doesn't implement > > > any xattrs other than POSIX ACLs. That's not done today but is in > > > principle possible. A prior version introduced SB_I_XATTR to this end. > > > Instead of this affecting all filesystems let those filesystems that > > > explicitly disable xattrs for some inodes disable POSIX ACLs by raising > > > IOP_NOACL. > > > > I'm still a little confused about this, and also about > > inode_xattr_disable. More below: > > > > > - if (!(old->d_inode->i_opflags & IOP_XATTR) || > > > - !(new->d_inode->i_opflags & IOP_XATTR)) > > > + if (inode_xattr_disabled(old->d_inode) || > > > + inode_xattr_disabled(new->d_inode)) > > > > This code shouldn't care about ACLs because the copy up itself > > should be all based around the ACL API, no? > > The loop copies up all xattrs. It retrieves all xattrs via > vfs_listxattr() then walks through all of them and copies them up. But > it's nothing that we couldn't work around if it buys as less headaches > overall. > > > > > > + if (!(inode->i_opflags & IOP_NOACL)) > > > error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); > > > else if (unlikely(is_bad_inode(inode))) > > > error = -EIO; > > > @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry, > > > if (error) > > > goto out_inode_unlock; > > > > > > - if (inode->i_opflags & IOP_XATTR) > > > + if (!(inode->i_opflags & IOP_NOACL)) > > > error = set_posix_acl(mnt_userns, dentry, acl_type, NULL); > > > > And here the lack of get/set methods should be all we need unless > > I'm missing something? > > For setting acl that should work, yes. > > > > > > diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c > > > index c7d1fa526dea..2a7037b165f0 100644 > > > --- a/fs/reiserfs/inode.c > > > +++ b/fs/reiserfs/inode.c > > > @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, > > > */ > > > if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { > > > inode->i_flags |= S_PRIVATE; > > > - inode->i_opflags &= ~IOP_XATTR; > > > + inode_xattr_disable(inode); > > > > I'll need to hear from the reiserfs maintainers, but this also seems > > like something that would be better done based on method presence. > > I mean, since this is locked I would think we could just: > > inode->i_op->{g,s}et_acl = NULL > > and for btrfs it should work to as it uses simple_dir_inode_operations > which doesn't implement get/set posix acl methods. > > > > > > diff --git a/fs/xattr.c b/fs/xattr.c > > > index adab9a70b536..89b6c122056d 100644 > > > --- a/fs/xattr.c > > > +++ b/fs/xattr.c > > > @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > > > error = security_inode_listxattr(dentry); > > > if (error) > > > return error; > > > - if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) { > > > + if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) { > > > error = inode->i_op->listxattr(dentry, list, size); > > > > So once listing ACLs is moved out of ->listxattr there should be no > > need to check anything for ACLs here either. > > I think so... > > But that would mean we'd need to change the ->listxattr() inode > operation to not return POSIX ACLs anymore. Instead vfs_listxattr() > would issue two vfs_get_acl() calls to check whether POSIX ACLs are So I see a few issues with this: * Network filesystems like 9p or cifs retrieve xattrs for ->listxattr() in a batch from the server and dump them into the provided buffer. If we want to stop listing POSIX ACLs in ->listxattr() that would mean we would need to filter them out of the buffer for such filesystems. From a cursory glance this might affect more than just 9p and cifs. * The fuse_listxattr() implementation has different permission requirements than fuse_get_acl() which would mean we would potentially refuse to list POSIX ACLs where we would have before or vica versa. * We risk losing returning a consistent snapshot of all xattr names for a given inode if we split ->listxattr() and POSIX ACLs apart. So I'm not sure that we can do it this way.
On Tue, Jan 31, 2023 at 12:36:47PM +0100, Christian Brauner wrote: > On Mon, Jan 30, 2023 at 07:09:09PM +0100, Christian Brauner wrote: > > On Mon, Jan 30, 2023 at 05:50:53PM +0100, Christoph Hellwig wrote: > > > On Mon, Jan 30, 2023 at 05:41:57PM +0100, Christian Brauner wrote: > > > > The POSIX ACL api doesn't use the xattr handler infrastructure anymore. > > > > If we keep relying on IOP_XATTR we will have to find a way to raise > > > > IOP_XATTR during inode_init_always() if a filesystem doesn't implement > > > > any xattrs other than POSIX ACLs. That's not done today but is in > > > > principle possible. A prior version introduced SB_I_XATTR to this end. > > > > Instead of this affecting all filesystems let those filesystems that > > > > explicitly disable xattrs for some inodes disable POSIX ACLs by raising > > > > IOP_NOACL. > > > > > > I'm still a little confused about this, and also about > > > inode_xattr_disable. More below: > > > > > > > - if (!(old->d_inode->i_opflags & IOP_XATTR) || > > > > - !(new->d_inode->i_opflags & IOP_XATTR)) > > > > + if (inode_xattr_disabled(old->d_inode) || > > > > + inode_xattr_disabled(new->d_inode)) > > > > > > This code shouldn't care about ACLs because the copy up itself > > > should be all based around the ACL API, no? > > > > The loop copies up all xattrs. It retrieves all xattrs via > > vfs_listxattr() then walks through all of them and copies them up. But > > it's nothing that we couldn't work around if it buys as less headaches > > overall. > > > > > > > > > + if (!(inode->i_opflags & IOP_NOACL)) > > > > error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); > > > > else if (unlikely(is_bad_inode(inode))) > > > > error = -EIO; > > > > @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry, > > > > if (error) > > > > goto out_inode_unlock; > > > > > > > > - if (inode->i_opflags & IOP_XATTR) > > > > + if (!(inode->i_opflags & IOP_NOACL)) > > > > error = set_posix_acl(mnt_userns, dentry, acl_type, NULL); > > > > > > And here the lack of get/set methods should be all we need unless > > > I'm missing something? > > > > For setting acl that should work, yes. > > > > > > > > > diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c > > > > index c7d1fa526dea..2a7037b165f0 100644 > > > > --- a/fs/reiserfs/inode.c > > > > +++ b/fs/reiserfs/inode.c > > > > @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, > > > > */ > > > > if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { > > > > inode->i_flags |= S_PRIVATE; > > > > - inode->i_opflags &= ~IOP_XATTR; > > > > + inode_xattr_disable(inode); > > > > > > I'll need to hear from the reiserfs maintainers, but this also seems > > > like something that would be better done based on method presence. > > > > I mean, since this is locked I would think we could just: > > > > inode->i_op->{g,s}et_acl = NULL > > > > and for btrfs it should work to as it uses simple_dir_inode_operations > > which doesn't implement get/set posix acl methods. > > > > > > > > > diff --git a/fs/xattr.c b/fs/xattr.c > > > > index adab9a70b536..89b6c122056d 100644 > > > > --- a/fs/xattr.c > > > > +++ b/fs/xattr.c > > > > @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > > > > error = security_inode_listxattr(dentry); > > > > if (error) > > > > return error; > > > > - if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) { > > > > + if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) { > > > > error = inode->i_op->listxattr(dentry, list, size); > > > > > > So once listing ACLs is moved out of ->listxattr there should be no > > > need to check anything for ACLs here either. > > > > I think so... > > > > But that would mean we'd need to change the ->listxattr() inode > > operation to not return POSIX ACLs anymore. Instead vfs_listxattr() > > would issue two vfs_get_acl() calls to check whether POSIX ACLs are > > So I see a few issues with this: > * Network filesystems like 9p or cifs retrieve xattrs for ->listxattr() > in a batch from the server and dump them into the provided buffer. > If we want to stop listing POSIX ACLs in ->listxattr() that would mean > we would need to filter them out of the buffer for such filesystems. > From a cursory glance this might affect more than just 9p and cifs. > * The fuse_listxattr() implementation has different permission > requirements than fuse_get_acl() which would mean we would potentially > refuse to list POSIX ACLs where we would have before or vica versa. > * We risk losing returning a consistent snapshot of all xattr names for > a given inode if we split ->listxattr() and POSIX ACLs apart. > > So I'm not sure that we can do it this way. So I've experimented a bit. Really, the remaining issue to remove the dependency of POSIX ACLs on IOP_XATTR is the check in vfs_listxattr() for IOP_XATTR. Removing IOP_XATTR from vfs_listxattr() from is really only a problem if there is any filesystems that removes IOP_XATTR without also assigning a dedicated set of inode_operations that either leaves ->listxattr() unimplement or makes it a NOP. The only filesystems for which this is true is reiserfs. But I believe we can fix this by forcing reiserfs to use a dedicated set of inode operations with ->listxattr(), and the POSIX ACL inode operations unimplemented. So here's what I have which would allows us to proceed with the removal. @Christoph, do you think that's doable or is there anything I'm still missing?: From 765c56cba40fb42e7e7a319cf3cbcc9d5abd7c11 Mon Sep 17 00:00:00 2001 From: Christian Brauner <brauner@kernel.org> Date: Tue, 31 Jan 2023 15:13:53 +0100 Subject: [PATCH 1/3] reiserfs: use simplify IOP_XATTR handling Reiserfs is the only filesystem that removes IOP_XATTR without also using a set of dedicated inode operations at the same time that nop all xattr related inode operations. This means we need to have a IOP_XATTR check in vfs_listxattr() instead of just being able to check for ->listxattr() being implemented. Introduce a dedicated set of nop inode operations that are used when IOP_XATTR is removed allowing us to remove that check from vfs_listxattr(). Cc: reiserfs-devel@vger.kernel.org Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- fs/reiserfs/inode.c | 1 + fs/reiserfs/namei.c | 17 +++++++++++++++++ fs/reiserfs/reiserfs.h | 1 + fs/reiserfs/xattr.c | 3 +++ 4 files changed, 22 insertions(+) diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index c7d1fa526dea..e293eaaed185 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -2090,6 +2090,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { inode->i_flags |= S_PRIVATE; inode->i_opflags &= ~IOP_XATTR; + inode->i_op = &reiserfs_privdir_inode_operations; } if (reiserfs_posixacl(inode->i_sb)) { diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c index 0b8aa99749f1..19aca1684fd1 100644 --- a/fs/reiserfs/namei.c +++ b/fs/reiserfs/namei.c @@ -384,6 +384,7 @@ static struct dentry *reiserfs_lookup(struct inode *dir, struct dentry *dentry, if (IS_PRIVATE(dir)) { inode->i_flags |= S_PRIVATE; inode->i_opflags &= ~IOP_XATTR; + inode->i_op = &reiserfs_privdir_inode_operations; } } reiserfs_write_unlock(dir->i_sb); @@ -1669,6 +1670,22 @@ const struct inode_operations reiserfs_dir_inode_operations = { .fileattr_set = reiserfs_fileattr_set, }; +const struct inode_operations reiserfs_privdir_inode_operations = { + .create = reiserfs_create, + .lookup = reiserfs_lookup, + .link = reiserfs_link, + .unlink = reiserfs_unlink, + .symlink = reiserfs_symlink, + .mkdir = reiserfs_mkdir, + .rmdir = reiserfs_rmdir, + .mknod = reiserfs_mknod, + .rename = reiserfs_rename, + .setattr = reiserfs_setattr, + .permission = reiserfs_permission, + .fileattr_get = reiserfs_fileattr_get, + .fileattr_set = reiserfs_fileattr_set, +}; + /* * symlink operations.. same as page_symlink_inode_operations, with xattr * stuff added diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h index 14726fd353c4..9d3a9c0df43b 100644 --- a/fs/reiserfs/reiserfs.h +++ b/fs/reiserfs/reiserfs.h @@ -3160,6 +3160,7 @@ static inline int reiserfs_proc_info_global_done(void) /* dir.c */ extern const struct inode_operations reiserfs_dir_inode_operations; +extern const struct inode_operations reiserfs_privdir_inode_operations; extern const struct inode_operations reiserfs_symlink_inode_operations; extern const struct inode_operations reiserfs_special_inode_operations; extern const struct file_operations reiserfs_dir_operations; diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c index 1864a35853a9..01dc07fb60a4 100644 --- a/fs/reiserfs/xattr.c +++ b/fs/reiserfs/xattr.c @@ -912,6 +912,8 @@ static int create_privroot(struct dentry *dentry) d_inode(dentry)->i_flags |= S_PRIVATE; d_inode(dentry)->i_opflags &= ~IOP_XATTR; + d_inode(dentry)->i_op = &reiserfs_privdir_inode_operations; + reiserfs_info(dentry->d_sb, "Created %s - reserved for xattr " "storage.\n", PRIVROOT_NAME); @@ -984,6 +986,7 @@ int reiserfs_lookup_privroot(struct super_block *s) if (d_really_is_positive(dentry)) { d_inode(dentry)->i_flags |= S_PRIVATE; d_inode(dentry)->i_opflags &= ~IOP_XATTR; + d_inode(dentry)->i_op = &reiserfs_privdir_inode_operations; } } else err = PTR_ERR(dentry);
Sorry for not keeping up with your flow of ideas, so chiming in now: > diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c > index c7d1fa526dea..e293eaaed185 100644 > --- a/fs/reiserfs/inode.c > +++ b/fs/reiserfs/inode.c > @@ -2090,6 +2090,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, > if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { > inode->i_flags |= S_PRIVATE; > inode->i_opflags &= ~IOP_XATTR; > + inode->i_op = &reiserfs_privdir_inode_operations; I wonder if there is any way to set this where reiserfs assigns the other ops. > +const struct inode_operations reiserfs_privdir_inode_operations = { > + .create = reiserfs_create, > + .lookup = reiserfs_lookup, > + .link = reiserfs_link, > + .unlink = reiserfs_unlink, > + .symlink = reiserfs_symlink, > + .mkdir = reiserfs_mkdir, > + .rmdir = reiserfs_rmdir, > + .mknod = reiserfs_mknod, > + .rename = reiserfs_rename, > + .setattr = reiserfs_setattr, > + .permission = reiserfs_permission, > + .fileattr_get = reiserfs_fileattr_get, > + .fileattr_set = reiserfs_fileattr_set, > +}; I suspect many other ops aren't need either, but I really need input from people that known reiserfs better. > + if (likely(!is_bad_inode(inode))) > error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); > else > + error = -EIO; I wonder if the is_bad_inode check should simplify move into get/set_posix_acl. But otherwise this patch looks good.
diff --git a/fs/bad_inode.c b/fs/bad_inode.c index 92737166203f..524e5e35b5d6 100644 --- a/fs/bad_inode.c +++ b/fs/bad_inode.c @@ -16,6 +16,7 @@ #include <linux/namei.h> #include <linux/poll.h> #include <linux/fiemap.h> +#include <linux/xattr.h> static int bad_file_open(struct inode *inode, struct file *filp) { @@ -212,7 +213,7 @@ void make_bad_inode(struct inode *inode) inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); inode->i_op = &bad_inode_ops; - inode->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(inode); inode->i_fop = &bad_file_ops; } EXPORT_SYMBOL(make_bad_inode); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 98a800b8bd43..c015e554d186 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -5840,7 +5840,7 @@ static struct inode *new_simple_dir(struct super_block *s, * associated with the dentry */ inode->i_op = &simple_dir_inode_operations; - inode->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(inode); inode->i_fop = &simple_dir_operations; inode->i_mode = S_IFDIR | S_IRUGO | S_IWUSR | S_IXUGO; inode->i_mtime = current_time(inode); diff --git a/fs/libfs.c b/fs/libfs.c index aada4e7c8713..78052d91d60f 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -23,6 +23,7 @@ #include <linux/fsnotify.h> #include <linux/unicode.h> #include <linux/fscrypt.h> +#include <linux/xattr.h> #include <linux/uaccess.h> @@ -1375,7 +1376,7 @@ void make_empty_dir_inode(struct inode *inode) inode->i_blocks = 0; inode->i_op = &empty_dir_inode_operations; - inode->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(inode); inode->i_fop = &empty_dir_operations; } diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index c14e90764e35..0b48f0aa9558 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -81,8 +81,8 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de int error = 0; size_t slen; - if (!(old->d_inode->i_opflags & IOP_XATTR) || - !(new->d_inode->i_opflags & IOP_XATTR)) + if (inode_xattr_disabled(old->d_inode) || + inode_xattr_disabled(new->d_inode)) return 0; list_size = vfs_listxattr(old, NULL, 0); diff --git a/fs/posix_acl.c b/fs/posix_acl.c index d7bc81fc0840..315d3926a13a 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -1095,7 +1095,7 @@ int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, if (error) goto out_inode_unlock; - if (inode->i_opflags & IOP_XATTR) + if (!(inode->i_opflags & IOP_NOACL)) error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); else if (unlikely(is_bad_inode(inode))) error = -EIO; @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry, if (error) goto out_inode_unlock; - if (inode->i_opflags & IOP_XATTR) + if (!(inode->i_opflags & IOP_NOACL)) error = set_posix_acl(mnt_userns, dentry, acl_type, NULL); else if (unlikely(is_bad_inode(inode))) error = -EIO; diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c index c7d1fa526dea..2a7037b165f0 100644 --- a/fs/reiserfs/inode.c +++ b/fs/reiserfs/inode.c @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th, */ if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) { inode->i_flags |= S_PRIVATE; - inode->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(inode); } if (reiserfs_posixacl(inode->i_sb)) { diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c index 0b8aa99749f1..4e2f121d6819 100644 --- a/fs/reiserfs/namei.c +++ b/fs/reiserfs/namei.c @@ -378,12 +378,12 @@ static struct dentry *reiserfs_lookup(struct inode *dir, struct dentry *dentry, /* * Propagate the private flag so we know we're - * in the priv tree. Also clear IOP_XATTR + * in the priv tree. Also clear xattrs * since we don't have xattrs on xattr files. */ if (IS_PRIVATE(dir)) { inode->i_flags |= S_PRIVATE; - inode->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(inode); } } reiserfs_write_unlock(dir->i_sb); diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c index 8b2d52443f41..2c326b57d4bc 100644 --- a/fs/reiserfs/xattr.c +++ b/fs/reiserfs/xattr.c @@ -889,7 +889,7 @@ static int create_privroot(struct dentry *dentry) } d_inode(dentry)->i_flags |= S_PRIVATE; - d_inode(dentry)->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(d_inode(dentry)); reiserfs_info(dentry->d_sb, "Created %s - reserved for xattr " "storage.\n", PRIVROOT_NAME); @@ -977,7 +977,7 @@ int reiserfs_lookup_privroot(struct super_block *s) d_set_d_op(dentry, &xattr_lookup_poison_ops); if (d_really_is_positive(dentry)) { d_inode(dentry)->i_flags |= S_PRIVATE; - d_inode(dentry)->i_opflags &= ~IOP_XATTR; + inode_xattr_disable(d_inode(dentry)); } } else err = PTR_ERR(dentry); diff --git a/fs/xattr.c b/fs/xattr.c index adab9a70b536..89b6c122056d 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) error = security_inode_listxattr(dentry); if (error) return error; - if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) { + if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) { error = inode->i_op->listxattr(dentry, list, size); } else { error = security_inode_listsecurity(inode, list, size); diff --git a/include/linux/fs.h b/include/linux/fs.h index c1769a2c5d70..f4cbac68598a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -582,6 +582,7 @@ is_uncached_acl(struct posix_acl *acl) #define IOP_NOFOLLOW 0x0004 #define IOP_XATTR 0x0008 #define IOP_DEFAULT_READLINK 0x0010 +#define IOP_NOACL 0x0020 struct fsnotify_mark_connector; diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 2e7dd44926e4..23bbe98cfc16 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -109,5 +109,16 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, char *buffer, size_t size); void simple_xattr_add(struct simple_xattrs *xattrs, struct simple_xattr *new_xattr); +static inline void inode_xattr_disable(struct inode *inode) +{ + inode->i_opflags &= ~IOP_XATTR; + inode->i_opflags |= IOP_NOACL; +} + +static inline bool inode_xattr_disabled(struct inode *inode) +{ + return !(inode->i_opflags & IOP_XATTR) && + (inode->i_opflags & IOP_NOACL); +} #endif /* _LINUX_XATTR_H */
The POSIX ACL api doesn't use the xattr handler infrastructure anymore. If we keep relying on IOP_XATTR we will have to find a way to raise IOP_XATTR during inode_init_always() if a filesystem doesn't implement any xattrs other than POSIX ACLs. That's not done today but is in principle possible. A prior version introduced SB_I_XATTR to this end. Instead of this affecting all filesystems let those filesystems that explicitly disable xattrs for some inodes disable POSIX ACLs by raising IOP_NOACL. Link: https://lore.kernel.org/linux-ext4/20230130091615.GB5178@lst.de Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- Changes in v2: - Patch introduced. --- fs/bad_inode.c | 3 ++- fs/btrfs/inode.c | 2 +- fs/libfs.c | 3 ++- fs/overlayfs/copy_up.c | 4 ++-- fs/posix_acl.c | 4 ++-- fs/reiserfs/inode.c | 2 +- fs/reiserfs/namei.c | 4 ++-- fs/reiserfs/xattr.c | 4 ++-- fs/xattr.c | 2 +- include/linux/fs.h | 1 + include/linux/xattr.h | 11 +++++++++++ 11 files changed, 27 insertions(+), 13 deletions(-)