Re: [PATCH] xfs: do not clear S_ISUID|S_ISGID for idmapped mounts

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Feb 25, 2022 at 01:42:26PM +0300, Andrey Zhadchenko wrote:
> 
> 
> On 2/25/22 12:45, Christian Brauner wrote:
> > On Thu, Feb 24, 2022 at 05:57:38PM -0800, Darrick J. Wong wrote:
> > > On Mon, Feb 21, 2022 at 09:22:18PM +0300, Andrey Zhadchenko wrote:
> > > > xfs_fileattr_set() handles idmapped mounts correctly and do not drop this
> > > > bits.
> > > > Unfortunately chown syscall results in different callstask:
> > > > i_op->xfs_vn_setattr()->...->xfs_setattr_nonsize() which checks if process
> > > > has CAP_FSETID capable in init_user_ns rather than mntns userns.
> > > > 
> > > > Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> > > 
> > > LGTM...
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > 
> > Darrick, could I ask you to please wait with applying.
> > The correct fix for this is either to simply remove the check here
> > altogether as we figured out in the thread or to switch to a generic vfs
> > helper setattr_copy().
> > Andrey will send a new patch in the not too distant future afaict
> > including tests.
> 
> Yes, please do not apply this patch for now. I am currently working on next
> version, however it is postponed a bit due to my personal affairs.
> Also I intend to add a second patch for xfs_fileattr_set() since it has the
> similar flaw - it may drop S_ISUID|S_ISGID for directories and may not drop
> S_ISUID for executable files.
> I expect to send patches next week alongside with new xfstests.

Ah, fair enough.  Felipe noticed that generic/673 produced inconsistent
outputs between btrfs and xfs last week and had asked about why the
setuid/setgid dropping behavior was unique to xfs.  We discovered that
xfs' omission of the setattr_copy logic was behind that...

--D