[v1] selftests/mount_setattr: fix idmap_mount_tree_invalid failed to run

[PATCH] selftests/mount_setattr: fix idmap_mount_tree_invalid failed to run

Posted by zhouyuhang 1 month ago

From: zhouyuhang <zhouyuhang@kylinos.cn>

Test case idmap_mount_tree_invalid failed to run on the newer kernel
with the following output:

 #  RUN           mount_setattr_idmapped.idmap_mount_tree_invalid ...
 # mount_setattr_test.c:1428:idmap_mount_tree_invalid:Expected sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr,  sizeof(attr)) (0) ! = 0 (0)
 # idmap_mount_tree_invalid: Test terminated by assertion

This is because tmpfs is mounted at "/mnt/A", and tmpfs already
contains the flag FS_ALLOW_IDMAP after the commit 7a80e5b8c6fa ("shmem:
support idmapped mounts for tmpfs"). So calling sys_mount_setattr here
returns 0 instead of -EINVAL as expected.

Ramfs is mounted at "/mnt/B" and does not support idmap mounts.
So we can use "/mnt/B" instead of "/mnt/A" to make the test run
successfully with the following output:

 # Starting 1 tests from 1 test cases.
 #  RUN           mount_setattr_idmapped.idmap_mount_tree_invalid ...
 #            OK  mount_setattr_idmapped.idmap_mount_tree_invalid
 ok 1 mount_setattr_idmapped.idmap_mount_tree_invalid
 # PASSED: 1 / 1 tests passed.

Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn>
---
 tools/testing/selftests/mount_setattr/mount_setattr_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index c6a8c732b802..54552c19bc24 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -1414,7 +1414,7 @@ TEST_F(mount_setattr_idmapped, idmap_mount_tree_invalid)
 	ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/b", 0, 0, 0), 0);
 	ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/BB/b", 0, 0, 0), 0);
 
-	open_tree_fd = sys_open_tree(-EBADF, "/mnt/A",
+	open_tree_fd = sys_open_tree(-EBADF, "/mnt/B",
 				     AT_RECURSIVE |
 				     AT_EMPTY_PATH |
 				     AT_NO_AUTOMOUNT |
-- 
2.27.0

Re: [PATCH] selftests/mount_setattr: fix idmap_mount_tree_invalid failed to run

Posted by Shuah Khan 1 month ago

On 10/24/24 03:50, zhouyuhang wrote:
> From: zhouyuhang <zhouyuhang@kylinos.cn>
> 
> Test case idmap_mount_tree_invalid failed to run on the newer kernel
> with the following output:
> 
>   #  RUN           mount_setattr_idmapped.idmap_mount_tree_invalid ...
>   # mount_setattr_test.c:1428:idmap_mount_tree_invalid:Expected sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr,  sizeof(attr)) (0) ! = 0 (0)
>   # idmap_mount_tree_invalid: Test terminated by assertion
> 
> This is because tmpfs is mounted at "/mnt/A", and tmpfs already
> contains the flag FS_ALLOW_IDMAP after the commit 7a80e5b8c6fa ("shmem:
> support idmapped mounts for tmpfs"). So calling sys_mount_setattr here
> returns 0 instead of -EINVAL as expected.
> 
> Ramfs is mounted at "/mnt/B" and does not support idmap mounts.
> So we can use "/mnt/B" instead of "/mnt/A" to make the test run
> successfully with the following output:
> 
>   # Starting 1 tests from 1 test cases.
>   #  RUN           mount_setattr_idmapped.idmap_mount_tree_invalid ...
>   #            OK  mount_setattr_idmapped.idmap_mount_tree_invalid
>   ok 1 mount_setattr_idmapped.idmap_mount_tree_invalid
>   # PASSED: 1 / 1 tests passed.
> 

Sounds like this code is testing this very condition passing
in invalid mount to see what happens. If that is the intent
this patch is incorrect.

> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn>
> ---
>   tools/testing/selftests/mount_setattr/mount_setattr_test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> index c6a8c732b802..54552c19bc24 100644
> --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> @@ -1414,7 +1414,7 @@ TEST_F(mount_setattr_idmapped, idmap_mount_tree_invalid)
>   	ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/b", 0, 0, 0), 0);
>   	ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/BB/b", 0, 0, 0), 0);
>   
> -	open_tree_fd = sys_open_tree(-EBADF, "/mnt/A",
> +	open_tree_fd = sys_open_tree(-EBADF, "/mnt/B",
>   				     AT_RECURSIVE |
>   				     AT_EMPTY_PATH |
>   				     AT_NO_AUTOMOUNT |

thanks,
-- Shuah

Re: [PATCH] selftests/mount_setattr: fix idmap_mount_tree_invalid failed to run

Posted by zhouyuhang 1 month ago


在 2024/10/24 22:26, Shuah Khan 写道:
> On 10/24/24 03:50, zhouyuhang wrote:
>> From: zhouyuhang <zhouyuhang@kylinos.cn>
>>
>> Test case idmap_mount_tree_invalid failed to run on the newer kernel
>> with the following output:
>>
>>   #  RUN mount_setattr_idmapped.idmap_mount_tree_invalid ...
>>   # mount_setattr_test.c:1428:idmap_mount_tree_invalid:Expected 
>> sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, 
>> sizeof(attr)) (0) ! = 0 (0)
>>   # idmap_mount_tree_invalid: Test terminated by assertion
>>
>> This is because tmpfs is mounted at "/mnt/A", and tmpfs already
>> contains the flag FS_ALLOW_IDMAP after the commit 7a80e5b8c6fa ("shmem:
>> support idmapped mounts for tmpfs"). So calling sys_mount_setattr here
>> returns 0 instead of -EINVAL as expected.
>>
>> Ramfs is mounted at "/mnt/B" and does not support idmap mounts.
>> So we can use "/mnt/B" instead of "/mnt/A" to make the test run
>> successfully with the following output:
>>
>>   # Starting 1 tests from 1 test cases.
>>   #  RUN mount_setattr_idmapped.idmap_mount_tree_invalid ...
>>   #            OK mount_setattr_idmapped.idmap_mount_tree_invalid
>>   ok 1 mount_setattr_idmapped.idmap_mount_tree_invalid
>>   # PASSED: 1 / 1 tests passed.
>>
>
> Sounds like this code is testing this very condition passing
> in invalid mount to see what happens. If that is the intent
> this patch is incorrect.
>

I think I probably understand what you mean, what you're saying is that 
the output of this line of errors is the condition,
and the main purpose of the test case is to see what happens when it 
invalid mount. But it's valid now, isn't it?
So we need to fix it. I don't think that constructing this error with 
ramfs will have any impact on the code that follows.
If you feel that using "/mnt/B" is unreliable, I think we can 
temporarily mount ramfs to "/mnt/A" here and continue using "/mnt/A".
Do you think this is feasible? Looking forward to your reply, thank you.

>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn>
>> ---
>>   tools/testing/selftests/mount_setattr/mount_setattr_test.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git 
>> a/tools/testing/selftests/mount_setattr/mount_setattr_test.c 
>> b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>> index c6a8c732b802..54552c19bc24 100644
>> --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>> +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>> @@ -1414,7 +1414,7 @@ TEST_F(mount_setattr_idmapped, 
>> idmap_mount_tree_invalid)
>>       ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/b", 0, 0, 0), 0);
>>       ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/BB/b", 0, 0, 0), 0);
>>   -    open_tree_fd = sys_open_tree(-EBADF, "/mnt/A",
>> +    open_tree_fd = sys_open_tree(-EBADF, "/mnt/B",
>>                        AT_RECURSIVE |
>>                        AT_EMPTY_PATH |
>>                        AT_NO_AUTOMOUNT |
>
> thanks,
> -- Shuah

Re: [PATCH] selftests/mount_setattr: fix idmap_mount_tree_invalid failed to run

Posted by Shuah Khan 4 weeks, 1 day ago

On 10/25/24 02:08, zhouyuhang wrote:
> 
> 
> 在 2024/10/24 22:26, Shuah Khan 写道:
>> On 10/24/24 03:50, zhouyuhang wrote:
>>> From: zhouyuhang <zhouyuhang@kylinos.cn>
>>>
>>> Test case idmap_mount_tree_invalid failed to run on the newer kernel
>>> with the following output:
>>>
>>>   #  RUN mount_setattr_idmapped.idmap_mount_tree_invalid ...
>>>   # mount_setattr_test.c:1428:idmap_mount_tree_invalid:Expected sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr)) (0) ! = 0 (0)
>>>   # idmap_mount_tree_invalid: Test terminated by assertion
>>>
>>> This is because tmpfs is mounted at "/mnt/A", and tmpfs already
>>> contains the flag FS_ALLOW_IDMAP after the commit 7a80e5b8c6fa ("shmem:
>>> support idmapped mounts for tmpfs"). So calling sys_mount_setattr here
>>> returns 0 instead of -EINVAL as expected.
>>>
>>> Ramfs is mounted at "/mnt/B" and does not support idmap mounts.
>>> So we can use "/mnt/B" instead of "/mnt/A" to make the test run
>>> successfully with the following output:
>>>
>>>   # Starting 1 tests from 1 test cases.
>>>   #  RUN mount_setattr_idmapped.idmap_mount_tree_invalid ...
>>>   #            OK mount_setattr_idmapped.idmap_mount_tree_invalid
>>>   ok 1 mount_setattr_idmapped.idmap_mount_tree_invalid
>>>   # PASSED: 1 / 1 tests passed.
>>>
>>
>> Sounds like this code is testing this very condition passing
>> in invalid mount to see what happens. If that is the intent
>> this patch is incorrect.
>>
> 
> I think I probably understand what you mean, what you're saying is that the output of this line of errors is the condition,
> and the main purpose of the test case is to see what happens when it invalid mount. But it's valid now, isn't it?
> So we need to fix it. I don't think that constructing this error with ramfs will have any impact on the code that follows.
> If you feel that using "/mnt/B" is unreliable, I think we can temporarily mount ramfs to "/mnt/A" here and continue using "/mnt/A".
> Do you think this is feasible? Looking forward to your reply, thank you.
> 

What I am saying is if this test is intended to test invalid mounts, passing
"/mnt/A" makes perfect sense.

>>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn>
>>> ---
>>>   tools/testing/selftests/mount_setattr/mount_setattr_test.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>>> index c6a8c732b802..54552c19bc24 100644
>>> --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>>> +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
>>> @@ -1414,7 +1414,7 @@ TEST_F(mount_setattr_idmapped, idmap_mount_tree_invalid)
>>>       ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/b", 0, 0, 0), 0);
>>>       ASSERT_EQ(expected_uid_gid(-EBADF, "/tmp/B/BB/b", 0, 0, 0), 0);
>>>   -    open_tree_fd = sys_open_tree(-EBADF, "/mnt/A",
>>> +    open_tree_fd = sys_open_tree(-EBADF, "/mnt/B",
>>>                        AT_RECURSIVE |
>>>                        AT_EMPTY_PATH |
>>>                        AT_NO_AUTOMOUNT |
>>
>> thanks,
>> -- Shuah
> 

thanks,
-- Shuah