Message-ID: <5c4272c4-3f9c-4ce8-ba73-50b5f16ab605@163.com>
Date: Tue, 15 Oct 2024 17:00:33 +0800
From: zhouyuhang <zhouyuhang1010@....com>
To: Shuah Khan <skhan@...uxfoundation.org>, brauner@...nel.org,
 shuah@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 zhouyuhang <zhouyuhang@...inos.cn>
Subject: Re: [PATCH] selftests: clone3: Use the capget and capset syscall
 directly


在 2024/10/15 06:38, Shuah Khan 写道:
> On 10/12/24 02:28, zhouyuhang wrote:
>>
>> On 2024/10/11 22:21, Shuah Khan wrote:
>>> On 10/11/24 00:59, zhouyuhang wrote:
>>>>
>>>> On 2024/10/10 23:50, Shuah Khan wrote:
>>>>> On 10/10/24 06:16, zhouyuhang wrote:
>>>>>> From: zhouyuhang <zhouyuhang@...inos.cn>
>>>>>>
>>>>>> The libcap commit aca076443591 ("Make cap_t operations thread 
>>>>>> safe.") added a
>>>>>> __u8 mutex at the beginning of the struct _cap_struct,it changes 
>>>>>> the offset of
>>>>>> the members in the structure that breaks the assumption made in 
>>>>>> the "struct libcap"
>>>>>> definition in clone3_cap_checkpoint_restore.c.So use the capget 
>>>>>> and capset syscall
>>>>>> directly and remove the libcap library dependency like the commit 
>>>>>> 663af70aabb7
>>>>>> ("bpf: selftests: Add helpers to directly use the capget and 
>>>>>> capset syscall") does.
>>>>>>
>>>>>
>>>>> NIT: grammar and comma spacing. Please fix those for readability.
>>>>> e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it"
>>>>> Fix others as well.
>>>>>
>>>>
>>>> Thanks, I'll fix it in V2
>>>>
>>>>
>>>>>> Signed-off-by: zhouyuhang <zhouyuhang@...inos.cn>
>>>>>> ---
>>>>>>   tools/testing/selftests/clone3/Makefile       |  1 -
>>>>>>   .../clone3/clone3_cap_checkpoint_restore.c    | 60 
>>>>>> +++++++++----------
>>>>>>   2 files changed, 28 insertions(+), 33 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/clone3/Makefile 
>>>>>> b/tools/testing/selftests/clone3/Makefile
>>>>>> index 84832c369a2e..59d26e8da8d2 100644
>>>>>> --- a/tools/testing/selftests/clone3/Makefile
>>>>>> +++ b/tools/testing/selftests/clone3/Makefile
>>>>>> @@ -1,6 +1,5 @@
>>>>>>   # SPDX-License-Identifier: GPL-2.0
>>>>>>   CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES)
>>>>>> -LDLIBS += -lcap
>>>>>>     TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
>>>>>>       clone3_cap_checkpoint_restore
>>>>>> diff --git 
>>>>>> a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c 
>>>>>> b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>>>>> index 3c196fa86c99..111912e2aead 100644
>>>>>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>>>>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>>>>> @@ -15,7 +15,7 @@
>>>>>>   #include <stdio.h>
>>>>>>   #include <stdlib.h>
>>>>>>   #include <stdbool.h>
>>>>>> -#include <sys/capability.h>
>>>>>> +#include <linux/capability.h>
>>>>>>   #include <sys/prctl.h>
>>>>>>   #include <sys/syscall.h>
>>>>>>   #include <sys/types.h>
>>>>>> @@ -27,6 +27,13 @@
>>>>>>   #include "../kselftest_harness.h"
>>>>>>   #include "clone3_selftests.h"
>>>>>>   +#ifndef CAP_CHECKPOINT_RESTORE
>>>>>> +#define CAP_CHECKPOINT_RESTORE 40
>>>>>> +#endif
>>>>>> +
>>>>>
>>>>> Why is this necessary? This is defined in linux/capability.h.

Sorry for not noticing this before.
This is to be compatible with some older versions of linux/capability.h 
that do not define this macro.

>>>>>
>>>>>> +int capget(cap_user_header_t header, cap_user_data_t data);
>>>>>> +int capset(cap_user_header_t header, const cap_user_data_t data);
>>>>>
>>>>> In general prototypes such as these should be defined in header
>>>>> file. Why are we defining these here?
>>>>>
>>>>> These are defined in sys/capability.h
>>>>>
>>>>> I don't understand this change. You are removing sys/capability.h
>>>>> which requires you to add these defines here. This doesn't
>>>>> sound like a correct solution to me.
>>>>>
>>>>
>>>> I tested it on my machine without libcap-dev installed, the 
>>>> /usr/include/linux/capability.h
>>>>
>>>> is on this machine by default. Successfully compiled using #include 
>>>> <linux/capability.h>
>>>>
>>>> but not with #include <sys/capability.h>. This patch removes libcap 
>>>> library dependencies.
>>>>
>>>> And we don't use any part of sys/capability.h other than these two 
>>>> syscalls. So I think that's why it's necessary.
>>>
>>> You are changing the code to not include sys/capability.h
>>> What happens if sys/capability.h along with linux/capability.h
>>>
>>> Do you see problems?
>>>
>>
>> I'm sorry, maybe I wasn't clear enough.
>> When we install the libcap library it will have the following output:
>>
>> test@...t:~/work/libcap$ sudo make install | grep capability
>> install -m 0644 include/sys/capability.h /usr/include/sys
>> install -m 0644 include/sys/capability.h /usr/include/sys
>> /usr/share/man/man5 capability.conf.5 \
>>
>> It installs sys/capability.h in the correct location, but does not
>>
>> install linux/capability.h, so sys/capability.h is bound to the 
>> libcap library
>
> It won't install inux/capability.h unless you run "make headers" in
> the kernel repo.
>
>>
>> and they will either exist or disappear together. Now I want to remove
>>
>> the dependency of the test on libcap library so I changed the code 
>> that it
>>
>> does not contain sys/capability.h but instead linux/capability.h,
>>
>> so that the test can compile successfully without libcap being 
>> installed,
>>
>> these two syscalls are not declared in linux/capability.h(It is 
>> sufficient for test use except for these two syscalls)
>>
>> so we need to declare them here. I think that's why the commit 
>> 663af70aabb7
>>
>> ("bpf: selftests: Add helpers to directly use the capget and capset 
>> syscall") I refered to
>>
>> does the same thing. As for your question "What happens if 
>> sys/capability.h along
>>
>> with linux/capability.h", I haven't found the problem yet, I 
>> sincerely hope you can
>>
>> help me improve this code. Thank you very much.
>
> Try this:
>
> Run make headers in the kernel repo.
> Build without making any changes.
> Then add you changes and add linux/capability.h to include files
>
> Tell me what happens.
>
> The change you are making isn't correct. Because you don't want to
> define system calls locally in your source file.
>

Thanks, I see.
Maybe I should move them to a new header file and resend a patch.

> thanks,
> -- Shuah

Powered by blists - more mailing lists