Message-ID: <0f79692e-ed68-462d-8ec7-955219116282@linuxfoundation.org>
Date: Mon, 14 Oct 2024 16:38:23 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: zhouyuhang <zhouyuhang1010@....com>, brauner@...nel.org, shuah@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 zhouyuhang <zhouyuhang@...inos.cn>, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] selftests: clone3: Use the capget and capset syscall
 directly

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.
>>>>
>>>>> +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,
-- Shuah

Powered by blists - more mailing lists