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.
