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; }
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");/* 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");
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
linux-kselftest-mirror@lists.linaro.org
-
Shuah Khan -
zhouyuhang