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.
