Re: [PATCH v2] selftests: clone3: Use the capget and capset syscall directly
From: zhouyuhang
Date: Wed Oct 16 2024 - 05:19:29 EST
在 2024/10/15 23:31, Shuah Khan 写道:
On 10/15/24 04:59, zhouyuhang wrote:
From: zhouyuhang <zhouyuhang@xxxxxxxxxx>
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.
This will make the test fail. 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@xxxxxxxxxx>
---
tools/testing/selftests/clone3/Makefile | 1 -
.../clone3/clone3_cap_checkpoint_restore.c | 53 ++++++++-----------
.../selftests/clone3/clone3_cap_helpers.h | 23 ++++++++
3 files changed, 44 insertions(+), 33 deletions(-)
create mode 100644 tools/testing/selftests/clone3/clone3_cap_helpers.h
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..242088eeec88 100644
--- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
+++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
@@ -15,7 +15,6 @@
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
-#include <sys/capability.h>
#include <sys/prctl.h>
#include <sys/syscall.h>
#include <sys/types.h>
@@ -26,6 +25,7 @@
#include "../kselftest_harness.h"
#include "clone3_selftests.h"
+#include "clone3_cap_helpers.h"
static void child_exit(int ret)
{
@@ -87,47 +87,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;
}
diff --git a/tools/testing/selftests/clone3/clone3_cap_helpers.h
b/tools/testing/selftests/clone3/clone3_cap_helpers.h
new file mode 100644
index 000000000000..3fa59ef68fb8
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_cap_helpers.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __CLONE3_CAP_HELPERS_H
+#define __CLONE3_CAP_HELPERS_H
+
+#include <linux/capability.h>
+
+/*
+ * Compatible with older version
+ * header file without defined
+ * CAP_CHECKPOINT_RESTORE.
+ */
+#ifndef CAP_CHECKPOINT_RESTORE
+#define CAP_CHECKPOINT_RESTORE 40
+#endif
+
+/*
+ * Removed the libcap library dependency.
+ * So declare them here directly.
+ */
+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);
Sorry you haven't addressed my comments on your v1 yet.
I repeat that this is not the right direction to define system
calls locally.
I got it. I am willing to modify the code so that syscalls are not
defined in local files,
but this would require including sys/capability.h which would not remove
the
dependency on the libcap library. So, should we directly use syscalls or
use the
libcap library function in the "set_capability" function, or do you have
a better way.
I'd like to refer to your advice.
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.
thanks,
-- Shuah
I tried this, here are my steps.
Firstly, I ran 'make headers' in the kernel repo and it was successful.
Then I wasn't quite sure which path you were referring to as' build ',
so I compiled and installed libcap, and also compiled test, all of which
were successful.
Afterwards, I applied my patch and the test was successfully built and
running.
I guess what you're trying to express may be that these system calls
have already
been defined in sys/capability, and those defined in the local file are
duplicated with it.
So I included sys/capability.h and linux/capability.h and defined the
system calls in the test,
but there were no errors.
I think there may be a problem with my operation, and I hope you can
point it out.