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

[Dave Chinner <david@fromorbit.com>]

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?

> 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.

Hence regardless of where the problem lies, a different fix will be
required to address it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com