Re: [PATCH v2 1/8] fs: don't use IOP_XATTR for posix acls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux