Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers

[Christoph Hellwig <hch@lst.de>]

On Thu, Feb 20, 2020 at 05:26:16PM -0800, Darrick J. Wong wrote:
> >  	to->di_format = from->di_format;
> > -	inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> 
> Hmm.  I'm not up on my userns-fu, but right now this is effectively:
> 
> inode->i_uid = make_kuid(&init_user_ns, be32_to_cpu(from->di_uid));
> 
> > -	inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> > +	i_uid_write(inode, be32_to_cpu(from->di_uid));
> 
> Whereas this is:
> 
> inode->i_uid = make_kuid(inode->i_sb->s_user_ns, be32_to_cpu(...));

Yes.  Which is intentional and mentioned in the commit log.

> 
> What happens if s_user_ns != init_user_ns?  Isn't this a behavior
> change?  Granted, it looks like many of the other filesystems use
> i_uid_write so maybe we're the ones who are doing it wrong...?

In that case the uid gets translated.  Which is intentional as it is
done everywhere else and XFS is the ugly ducking out that fails
to properly take the user_ns into account.

> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -67,10 +67,12 @@ xfs_acl_from_disk(
> >  
> >  		switch (acl_e->e_tag) {
> >  		case ACL_USER:
> > -			acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> > +			acl_e->e_uid = make_kuid(&init_user_ns,
> > +						 be32_to_cpu(ace->ae_id));
> 
> And I'm assuming that the "gross layering violation in the vfs xattr
> code" is why it's init_user_ns here?

Yes.  The generic xattr code checks if the attr is one of the ACL ones
in common code before calling into the fs and already translates them,
causing a giant mess.