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


Reply via email to