[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b2e02494-0f22-476e-bb79-f3a133b7fa07@linuxfoundation.org>
Date: Fri, 11 Oct 2024 08:21:04 -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/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?
thanks,
-- Shuah
Powered by blists - more mailing lists