On Fri, Feb 21, 2020 at 04:54:50PM +0100, Christoph Hellwig wrote:
> 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.
Ok, we were doing it wrong. Should this series have fixed that as the
first patch (so that we could push it into old kernels) followed by the
actual icdinode field removal?
(Granted nobody seems to have complained...)
> > > --- 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.
Got it.
--D
