Re: [PATCH v2 4/6] idmapped-mounts: Add umask(S_IXGRP) wrapper for setgid_create* cases

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

on 2022/4/7 23:12, Christian Brauner wrote:
> On Thu, Apr 07, 2022 at 08:09:33PM +0800, Yang Xu wrote:
>> Since stipping S_SIGID should check S_IXGRP, so umask it to check whether
>> works well.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 66 +++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index d2638c64..d6769f08 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -8031,6 +8031,27 @@ out:
>>   	return fret;
>>   }
>>
>> +static int setgid_create_umask(void)
>> +{
>> +	pid_t pid;
>> +
>> +	umask(S_IXGRP);
>
> Ok, this is migraine territory (not your fault ofc). So I think we want
> to not just wrap the umask() and setfacl() code around the existing
> setgid() tests. That's just so complex to reason about that the test
> just adds confusion if we just hack it into existing functions.
>
> Can you please add separate tests that don't just wrap existing tests
> via umask()+fork() and instead add dedicated umask()-based and
> acl()-based functions.
Ok, I understand.
>
> Do you have time to do that?
Yes, I have time to do this. Yesterday I hurriedly sent out this 
patchset in order to get more comments in this week.
>
> Right now it's confusing because there's an intricate relationship
> between acls and current_umask() and that needs to be mentioned in the
> respective tests.
>
> The current_umask() is stripped from the mode directly in the vfs if the
> filesystem either doesn't support acls or the filesystem has been
> mounted without posic acl support.
>
> If the filesystem does support acls then current_umask() stripping is
> deferred to posix_acl_create(). So when the filesystem calls
> posix_acl_create() and there are no acls set or not supported then
> current_umask() will be stripped.
>
> If the parent directory has a default acl then permissions are based off
> of that and current_umask() is ignored. Specifically, if the ACL has an
> ACL_MASK entry, the group permissions correspond to the permissions of
> the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
> group permissions correspond to the permissions of the ACL_GROUP_OBJ
> entry.
>
> Yes, it's confusing which is why we need to clearly give both acls and
> the umask() tests their separate functions and not just hack them into
> the existing functions.

Got it.
>
> As it stands the umask() and posix acl() tests only pass by accident
> because the filesystem you're testing on supports acls but doesn't strip
> the S_IXGRP bit. So the current_umask() is ignored and that's why the
> tests pass, I think. Otherwise they'd fail because they test the wrong
> thing.
Oh, yes.
>
> You can verify this by setting
> export MOUNT_OPTIONS='-o noacl'
> in your xfstests config.
>
> You'll see the test fail just like it should:
>
> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check generic/999
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 imp1-vm 5.18.0-rc1-ovl-7d354bcd37d1 #273 SMP PREEMPT_DYNAMIC Thu Apr 7 11:07:08 UTC 2022
> MKFS_OPTIONS  -- /dev/sda4
> MOUNT_OPTIONS -- -o noacl /dev/sda4 /mnt/scratch
>
> generic/999 2s ... [failed, exit status 1]- output mismatch (see /home/ubuntu/src/git/xfstests/results//generic/999.out.bad)
>      --- tests/generic/999.out   2022-04-07 12:48:18.948000000 +0000
>      +++ /home/ubuntu/src/git/xfstests/results//generic/999.out.bad      2022-04-07 14:19:28.517811054 +0000
>      @@ -1,2 +1,5 @@
>       QA output created by 999
>       Silence is golden
>      +idmapped-mounts.c: 8002: setgid_create - Success - failure: is_setgid
>      +idmapped-mounts.c: 8110: setgid_create_umask - Success - failure: setgid
>      +idmapped-mounts.c: 14428: run_test - No such file or directory - failure: create operations in directories with setgid bit set by umask(S_IXGRP)
>      ...
>      (Run 'diff -u /home/ubuntu/src/git/xfstests/tests/generic/999.out /home/ubuntu/src/git/xfstests/results//generic/999.out.bad'  to see the entire diff)
> Ran: generic/999
> Failures: generic/999
> Failed 1 of 1 tests
I will design separate function for umask and acl, but I doubut whether 
we also need to test them in in_userns and idmaped_in_user situation.

ps: I will put umask and acl patch as the 5th/6th the patchset because 
other patch only has small nits and easy to modify.

Best Regards
Yang Xu