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