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@xxxxxxxxxxx>
> ---
> 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.
Do you have time to do that?
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.
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.
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
