Message-ID: <714162ff-a2f8-4fc6-91af-0ecd6376bc7f@163.com>
Date: Sat, 12 Oct 2024 16:28:29 +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


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

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.


> thanks,
> -- Shuah

Powered by blists - more mailing lists