From: zhouyuhang <zhouyuhang@kylinos.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.

Signed-off-by: zhouyuhang <zhouyuhang@kylinos.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
+
+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);
+
 static void child_exit(int ret)
 {
 	fflush(stdout);
@@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct __test_metadata *_metadata,
 	return ret;
 }
 
-struct libcap {
-	struct __user_cap_header_struct hdr;
-	struct __user_cap_data_struct data[2];
-};
-
 static int set_capability(void)
 {
-	cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID };
-	struct libcap *cap;
-	int ret = -1;
-	cap_t caps;
-
-	caps = cap_get_proc();
-	if (!caps) {
-		perror("cap_get_proc");
+	struct __user_cap_data_struct data[2];
+	struct __user_cap_header_struct hdr = {
+		.version = _LINUX_CAPABILITY_VERSION_3,
+	};
+	__u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID;
+	__u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32);
+	int ret;
+
+	ret = capget(&hdr, data);
+	if (ret) {
+		perror("capget");
 		return -1;
 	}
 
 	/* Drop all capabilities */
-	if (cap_clear(caps)) {
-		perror("cap_clear");
-		goto out;
-	}
+	memset(&data, 0, sizeof(data));
 
-	cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
-	cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);
+	data[0].effective |= cap0;
+	data[0].permitted |= cap0;
 
-	cap = (struct libcap *) caps;
+	data[1].effective |= cap1;
+	data[1].permitted |= cap1;
 
-	/* 40 -> CAP_CHECKPOINT_RESTORE */
-	cap->data[1].effective |= 1 << (40 - 32);
-	cap->data[1].permitted |= 1 << (40 - 32);
-
-	if (cap_set_proc(caps)) {
-		perror("cap_set_proc");
-		goto out;
+	ret = capset(&hdr, data);
+	if (ret) {
+		perror("capset");
+		return -1;
 	}
-	ret = 0;
-out:
-	if (cap_free(caps))
-		perror("cap_free");
 	return ret;
 }
 
-- 
2.25.1

On 10/10/24 06:16, zhouyuhang wrote:
> From: zhouyuhang <zhouyuhang@kylinos.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.

> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.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.

> +
>   static void child_exit(int ret)
>   {
>   	fflush(stdout);
> @@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct __test_metadata *_metadata,
>   	return ret;
>   }
>   
> -struct libcap {
> -	struct __user_cap_header_struct hdr;
> -	struct __user_cap_data_struct data[2];
> -};
> -
>   static int set_capability(void)
>   {
> -	cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID };
> -	struct libcap *cap;
> -	int ret = -1;
> -	cap_t caps;
> -
> -	caps = cap_get_proc();
> -	if (!caps) {
> -		perror("cap_get_proc");
> +	struct __user_cap_data_struct data[2];
> +	struct __user_cap_header_struct hdr = {
> +		.version = _LINUX_CAPABILITY_VERSION_3,
> +	};
> +	__u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID;
> +	__u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32);
> +	int ret;
> +
> +	ret = capget(&hdr, data);
> +	if (ret) {
> +		perror("capget");
>   		return -1;
>   	}
>   
>   	/* Drop all capabilities */
> -	if (cap_clear(caps)) {
> -		perror("cap_clear");
> -		goto out;
> -	}
> +	memset(&data, 0, sizeof(data));
>   
> -	cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
> -	cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);
> +	data[0].effective |= cap0;
> +	data[0].permitted |= cap0;
>   
> -	cap = (struct libcap *) caps;
> +	data[1].effective |= cap1;
> +	data[1].permitted |= cap1;
>   
> -	/* 40 -> CAP_CHECKPOINT_RESTORE */
> -	cap->data[1].effective |= 1 << (40 - 32);
> -	cap->data[1].permitted |= 1 << (40 - 32);
> -
> -	if (cap_set_proc(caps)) {
> -		perror("cap_set_proc");
> -		goto out;
> +	ret = capset(&hdr, data);
> +	if (ret) {
> +		perror("capset");
> +		return -1;
>   	}
> -	ret = 0;
> -out:
> -	if (cap_free(caps))
> -		perror("cap_free");
>   	return ret;
>   }
>   

thanks,
-- Shuah

On 2024/10/10 23:50, Shuah Khan wrote:
> On 10/10/24 06:16, zhouyuhang wrote:
>> From: zhouyuhang <zhouyuhang@kylinos.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@kylinos.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.


>> +
>>   static void child_exit(int ret)
>>   {
>>       fflush(stdout);
>> @@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct 
>> __test_metadata *_metadata,
>>       return ret;
>>   }
>>   -struct libcap {
>> -    struct __user_cap_header_struct hdr;
>> -    struct __user_cap_data_struct data[2];
>> -};
>> -
>>   static int set_capability(void)
>>   {
>> -    cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID };
>> -    struct libcap *cap;
>> -    int ret = -1;
>> -    cap_t caps;
>> -
>> -    caps = cap_get_proc();
>> -    if (!caps) {
>> -        perror("cap_get_proc");
>> +    struct __user_cap_data_struct data[2];
>> +    struct __user_cap_header_struct hdr = {
>> +        .version = _LINUX_CAPABILITY_VERSION_3,
>> +    };
>> +    __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID;
>> +    __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32);
>> +    int ret;
>> +
>> +    ret = capget(&hdr, data);
>> +    if (ret) {
>> +        perror("capget");
>>           return -1;
>>       }
>>         /* Drop all capabilities */
>> -    if (cap_clear(caps)) {
>> -        perror("cap_clear");
>> -        goto out;
>> -    }
>> +    memset(&data, 0, sizeof(data));
>>   -    cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
>> -    cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);
>> +    data[0].effective |= cap0;
>> +    data[0].permitted |= cap0;
>>   -    cap = (struct libcap *) caps;
>> +    data[1].effective |= cap1;
>> +    data[1].permitted |= cap1;
>>   -    /* 40 -> CAP_CHECKPOINT_RESTORE */
>> -    cap->data[1].effective |= 1 << (40 - 32);
>> -    cap->data[1].permitted |= 1 << (40 - 32);
>> -
>> -    if (cap_set_proc(caps)) {
>> -        perror("cap_set_proc");
>> -        goto out;
>> +    ret = capset(&hdr, data);
>> +    if (ret) {
>> +        perror("capset");
>> +        return -1;
>>       }
>> -    ret = 0;
>> -out:
>> -    if (cap_free(caps))
>> -        perror("cap_free");
>>       return ret;
>>   }
>
> thanks,
> -- Shuah

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

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@kylinos.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@kylinos.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@test:~/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

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@kylinos.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@kylinos.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@test:~/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

在 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@kylinos.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@kylinos.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@test:~/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

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@test:~/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