diff mbox series

[v4,1/3] idmapped-mounts: Reset errno to zero before run_test

Message ID 1651928726-2263-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/3] idmapped-mounts: Reset errno to zero before run_test | expand

Commit Message

Yang Xu (Fujitsu) May 7, 2022, 1:05 p.m. UTC 
If we run case on old kernel that doesn't support mount_setattr and
then fail on our own function before call is_setgid/is_setuid function
to reset errno, run_test will print "Function not implement" error.

We also check whether system support user namespace, so reset errno to
zero after userns check.

Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v3->v4: move this reset step after sys_has_usersn()
 src/idmapped-mounts/idmapped-mounts.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Zorro Lang May 7, 2022, 1:33 p.m. UTC | #1 
On Sat, May 07, 2022 at 09:05:24PM +0800, Yang Xu wrote:
> If we run case on old kernel that doesn't support mount_setattr and
> then fail on our own function before call is_setgid/is_setuid function
> to reset errno, run_test will print "Function not implement" error.
> 
> We also check whether system support user namespace, so reset errno to
> zero after userns check.
> 
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

This patchset bring in a new failure [1] on kernel 5.18.0-0.rc4+. Before I push,
just confirm that is it as your expected, not a regression ?

Thanks,
Zorro

[1]
# ./check generic/633
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 hp-xxxxxxx-xx 5.18.0-0.rc4.20220427git46cf2c613f4b10e.35.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Apr 27 13:03:32 UTC 2022
MKFS_OPTIONS  -- -f /dev/sda3
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch

generic/633 14s ... [failed, exit status 1]- output mismatch (see /root/git/xfstests/results//generic/633.out.bad)
    --- tests/generic/633.out   2022-04-29 23:07:23.547501513 +0800
    +++ /root/git/xfstests/results//generic/633.out.bad 2022-05-07 21:20:40.205852662 +0800
    @@ -1,2 +1,4 @@
     QA output created by 633
     Silence is golden
    +idmapped-mounts.c: 8244: setgid_create_idmapped - No such file or directory - failure: linkat
    +idmapped-mounts.c: 14437: run_test - Success - failure: create operations in directories with setgid bit set on idmapped mounts
    ...
    (Run 'diff -u /root/git/xfstests/tests/generic/633.out /root/git/xfstests/results//generic/633.out.bad'  to see the entire diff)
Ran: generic/633
Failures: generic/633
Failed 1 of 1 tests

> v3->v4: move this reset step after sys_has_usersn()
>  src/idmapped-mounts/idmapped-mounts.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index ce3f73be..2e94bf71 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -14232,6 +14232,8 @@ int main(int argc, char *argv[])
>  		exit(EXIT_SUCCESS);
>  	}
>  	t_has_userns = sys_has_userns();
> +	/* don't copy ENOSYS errno to child process on older kernel */
> +	errno = 0;
>  
>  	stash_overflowuid();
>  	stash_overflowgid();
> -- 
> 2.31.1
>
Yang Xu (Fujitsu) May 7, 2022, 1:56 p.m. UTC | #2 
on 2022/5/7 21:33, Zorro Lang wrote:
> On Sat, May 07, 2022 at 09:05:24PM +0800, Yang Xu wrote:
>> If we run case on old kernel that doesn't support mount_setattr and
>> then fail on our own function before call is_setgid/is_setuid function
>> to reset errno, run_test will print "Function not implement" error.
>>
>> We also check whether system support user namespace, so reset errno to
>> zero after userns check.
>>
>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> This patchset bring in a new failure [1] on kernel 5.18.0-0.rc4+. Before I push,
> just confirm that is it as your expected, not a regression ?

No, it should all pass because I don't introduce umask or acl operations.

>
> Thanks,
> Zorro
>
> [1]
> # ./check generic/633
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 hp-xxxxxxx-xx 5.18.0-0.rc4.20220427git46cf2c613f4b10e.35.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Apr 27 13:03:32 UTC 2022
> MKFS_OPTIONS  -- -f /dev/sda3
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch
>
> generic/633 14s ... [failed, exit status 1]- output mismatch (see /root/git/xfstests/results//generic/633.out.bad)
>      --- tests/generic/633.out   2022-04-29 23:07:23.547501513 +0800
>      +++ /root/git/xfstests/results//generic/633.out.bad 2022-05-07 21:20:40.205852662 +0800
>      @@ -1,2 +1,4 @@
>       QA output created by 633
>       Silence is golden
>      +idmapped-mounts.c: 8244: setgid_create_idmapped - No such file or directory - failure: linkat
>      +idmapped-mounts.c: 14437: run_test - Success - failure: create operations in directories with setgid bit set on idmapped mounts

It is a copy-paste error. It shoud use open_tree_fd in line 8243 as below:
  if (linkat(tmpfile_fd, "", open_tree_fd, FILE3, AT_EMPTY_PATH))

Can you try replace t_dir1_fd with open_tree_fd for tmpfile  permanent 
in setgid_create_idmapped and setgid_create_idmapped_in_userns function?

Best Regards
Yang Xu

>      ...
>      (Run 'diff -u /root/git/xfstests/tests/generic/633.out /root/git/xfstests/results//generic/633.out.bad'  to see the entire diff)
> Ran: generic/633
> Failures: generic/633
> Failed 1 of 1 tests
>
>> v3->v4: move this reset step after sys_has_usersn()
>>   src/idmapped-mounts/idmapped-mounts.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index ce3f73be..2e94bf71 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -14232,6 +14232,8 @@ int main(int argc, char *argv[])
>>   		exit(EXIT_SUCCESS);
>>   	}
>>   	t_has_userns = sys_has_userns();
>> +	/* don't copy ENOSYS errno to child process on older kernel */
>> +	errno = 0;
>>
>>   	stash_overflowuid();
>>   	stash_overflowgid();
>> --
>> 2.31.1
>>
>
Yang Xu (Fujitsu) May 7, 2022, 2:23 p.m. UTC | #3 
on 2022/5/7 21:56, xuyang2018.jy@fujitsu.com wrote:
> on 2022/5/7 21:33, Zorro Lang wrote:
>> On Sat, May 07, 2022 at 09:05:24PM +0800, Yang Xu wrote:
>>> If we run case on old kernel that doesn't support mount_setattr and
>>> then fail on our own function before call is_setgid/is_setuid function
>>> to reset errno, run_test will print "Function not implement" error.
>>>
>>> We also check whether system support user namespace, so reset errno to
>>> zero after userns check.
>>>
>>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>
>> This patchset bring in a new failure [1] on kernel 5.18.0-0.rc4+. Before I push,
>> just confirm that is it as your expected, not a regression ?
> 
> No, it should all pass because I don't introduce umask or acl operations.
> 
>>
>> Thanks,
>> Zorro
>>
>> [1]
>> # ./check generic/633
>> FSTYP         -- xfs (non-debug)
>> PLATFORM      -- Linux/x86_64 hp-xxxxxxx-xx 5.18.0-0.rc4.20220427git46cf2c613f4b10e.35.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Apr 27 13:03:32 UTC 2022
>> MKFS_OPTIONS  -- -f /dev/sda3
>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch
>>
>> generic/633 14s ... [failed, exit status 1]- output mismatch (see /root/git/xfstests/results//generic/633.out.bad)
>>       --- tests/generic/633.out   2022-04-29 23:07:23.547501513 +0800
>>       +++ /root/git/xfstests/results//generic/633.out.bad 2022-05-07 21:20:40.205852662 +0800
>>       @@ -1,2 +1,4 @@
>>        QA output created by 633
>>        Silence is golden
>>       +idmapped-mounts.c: 8244: setgid_create_idmapped - No such file or directory - failure: linkat
>>       +idmapped-mounts.c: 14437: run_test - Success - failure: create operations in directories with setgid bit set on idmapped mounts
> 
> It is a copy-paste error. It shoud use open_tree_fd in line 8243 as below:
>    if (linkat(tmpfile_fd, "", open_tree_fd, FILE3, AT_EMPTY_PATH))
> 
> Can you try replace t_dir1_fd with open_tree_fd for tmpfile  permanent
> in setgid_create_idmapped and setgid_create_idmapped_in_userns function?

Sorry, this error because of this situation:

ENOENT AT_EMPTY_PATH was specified in flags, but the caller did not have
the CAP_DAC_READ_SEARCH capability.

So I think we just use AT_EMPTY_PATH in setgid_create and another two
functions still use old way(snprint). I will send a new patchset.

ps: I has verified this.

> 
> Best Regards
> Yang Xu
> 
>>       ...
>>       (Run 'diff -u /root/git/xfstests/tests/generic/633.out /root/git/xfstests/results//generic/633.out.bad'  to see the entire diff)
>> Ran: generic/633
>> Failures: generic/633
>> Failed 1 of 1 tests
>>
>>> v3->v4: move this reset step after sys_has_usersn()
>>>    src/idmapped-mounts/idmapped-mounts.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>>> index ce3f73be..2e94bf71 100644
>>> --- a/src/idmapped-mounts/idmapped-mounts.c
>>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>>> @@ -14232,6 +14232,8 @@ int main(int argc, char *argv[])
>>>    		exit(EXIT_SUCCESS);
>>>    	}
>>>    	t_has_userns = sys_has_userns();
>>> +	/* don't copy ENOSYS errno to child process on older kernel */
>>> +	errno = 0;
>>>
>>>    	stash_overflowuid();
>>>    	stash_overflowgid();
>>> --
>>> 2.31.1
>>>
>>
diff mbox series

Patch

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index ce3f73be..2e94bf71 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -14232,6 +14232,8 @@  int main(int argc, char *argv[])
 		exit(EXIT_SUCCESS);
 	}
 	t_has_userns = sys_has_userns();
+	/* don't copy ENOSYS errno to child process on older kernel */
+	errno = 0;
 
 	stash_overflowuid();
 	stash_overflowgid();