Re: [RFC RESEND] xfs: fix up non-directory creation in SGID directories when umask S_IXGRP

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/3/22 16:43, Dave Chinner wrote:
> On Tue, Mar 22, 2022 at 04:04:17PM +0800, Yang Xu wrote:
>> Petr reported a problem that S_ISGID bit was not clean when testing ltp
>> case create09[1] by using umask(077).
>
> Ok. So what is the failure message from the test?
>
> When did the test start failing? Is this a recent failure or
> something that has been around for years? If it's recent, what
> commit broke it?
You have known this.
>
>> It fails because xfs will call posix_acl_create before xfs_init_new_node
>> calls inode_init_owner, so S_IXGRP mode will be clear when enable CONFIG_XFS_POSIXACL
>> and doesn't set acl or default acl on dir, then inode_init_owner will not clear
>> S_ISGID bit.
>
> I don't really follow what you are saying is the problem here - the
> rule we are supposed to be following is not clear to me, nor how XFS
> is behaving contrary to the rule. Can you explain the rule (e.g.
> from the test failure results) rather than try to explain where the
> code goes wrong, please?
>
>> The calltrace as below:
>>
>> use inode_init_owner(mnt_userns, inode)
>> [  296.760675]  xfs_init_new_inode+0x10e/0x6c0
>> [  296.760678]  xfs_create+0x401/0x610
>> use posix_acl_create(dir,&mode,&default_acl,&acl);
>> [  296.760681]  xfs_generic_create+0x123/0x2e0
>> [  296.760684]  ? _raw_spin_unlock+0x16/0x30
>> [  296.760687]  path_openat+0xfb8/0x1210
>> [  296.760689]  do_filp_open+0xb4/0x120
>> [  296.760691]  ? file_tty_write.isra.31+0x203/0x340
>> [  296.760697]  ? __check_object_size+0x150/0x170
>> [  296.760699]  do_sys_openat2+0x242/0x310
>> [  296.760702]  do_sys_open+0x4b/0x80
>> [  296.760704]  do_syscall_64+0x3a/0x80
>>
>> Fix this is simple, we can call posix_acl_create after xfs_init_new_inode completed,
>> so inode_init_owner can clear S_ISGID bit correctly like ext4 or btrfs does.
>>
>> But commit e6a688c33238 ("xfs: initialise attr fork on inode create") has created
>> attr fork in advance according to acl, so a better solution is that moving these
>> functions into xfs_init_new_inode.
>
> No, you can't do that. Xattrs cannot be created within the
> transaction context of the create operation because they require
> their own transaction context to run under. We cannot nest
> transaction contexts in XFS, so the ACL and other security xattrs
> must be created after the inode create has completed.
>
> Commit e6a688c33238 only initialises the inode attribute fork in the
> create transaction rather than requiring a separate transaction to
> do it before the xattrs are then created. It does not allow xattrs
> to be created from within the create transaction context.
Thanks for your reply, now, I know this.

Best Regards
Yang Xu
>
> Hence regardless of where the problem lies, a different fix will be
> required to address it.
>
> Cheers,
>
> Dave.