Message-ID: <b82a25f8-e7a9-4a42-9a16-c2cda99feecd@linuxfoundation.org>
Date: Tue, 15 Oct 2024 09:23:40 -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>
Subject: Re: [PATCH] selftests: clone3: Use the capget and capset syscall
 directly

On 10/15/24 03:00, zhouyuhang wrote:
> 

[snip] for clarity.

>>>>>> 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.

Try the above first.

>>
>> 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.

No. Please see above. I would rather not see these defined at all
locally.

thanks,
-- Shuah

Powered by blists - more mailing lists