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@xxxxxxxxxxxxx> > > > > > > LGTM... > > > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > 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
