| Message ID | 1649333375-2599-4-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] idmapped-mount: split setgid test from test-core | expand |
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. 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
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
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); + pid = fork(); + if (pid < 0) + die("failure: fork"); + + if (pid == 0) { + if (setgid_create()) + die("failure: setgid"); + exit(EXIT_SUCCESS); + } + + if (wait_for_pid(pid)) + return -1; + else + return 0; +} + static int setgid_create_idmapped(void) { int fret = -1; @@ -8157,6 +8178,27 @@ out: return fret; } +static int setgid_create_idmapped_umask(void) +{ + pid_t pid; + + umask(S_IXGRP); + pid = fork(); + if (pid < 0) + die("failure: fork"); + + if (pid == 0) { + if (setgid_create_idmapped()) + die("failure: setgid"); + exit(EXIT_SUCCESS); + } + + if (wait_for_pid(pid)) + return -1; + else + return 0; +} + static int setgid_create_idmapped_in_userns(void) { int fret = -1; @@ -8492,6 +8534,27 @@ out: return fret; } +static int setgid_create_idmapped_in_userns_umask(void) +{ + pid_t pid; + + umask(S_IXGRP); + pid = fork(); + if (pid < 0) + die("failure: fork"); + + if (pid == 0) { + if (setgid_create_idmapped_in_userns()) + die("failure: setgid"); + exit(EXIT_SUCCESS); + } + + if (wait_for_pid(pid)) + return -1; + else + return 0; +} + #define PTR_TO_INT(p) ((int)((intptr_t)(p))) #define INT_TO_PTR(u) ((void *)((intptr_t)(u))) @@ -14100,8 +14163,11 @@ struct t_idmapped_mounts t_setattr_fix_968219708108[] = { struct t_idmapped_mounts t_setgid[] = { { setgid_create, false, "create operations in directories with setgid bit set", }, + { setgid_create_umask, false, "create operations in directories with setgid bit set by umask(S_IXGRP)", }, { setgid_create_idmapped, true, "create operations in directories with setgid bit set on idmapped mounts", }, + { setgid_create_idmapped_umask, true, "create operations in directories with setgid bit set on idmapped mounts by umask(S_IXGRP)", }, { setgid_create_idmapped_in_userns, true, "create operations in directories with setgid bit set on idmapped mounts in user namespace", }, + { setgid_create_idmapped_in_userns_umask, true, "create operations in directories with setgid bit set on idmapped mounts in user namespace by umask(S_IXGRP)", }, }; static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size)
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(+)