Re: [PATCH v3] selftests: clone3: Use the capget and capset syscall directly

On 10/29/24 20:50, 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. This will cause the test case to fail with the following output:

# RUN global.clone3_cap_checkpoint_restore ... # clone3() syscall supported # clone3_cap_checkpoint_restore.c:151:clone3_cap_checkpoint_restore:Child has PID 130508 cap_set_proc: Operation not permitted

Sounds like EPERM is returned here. What's the error number from cap_set_proc().

# clone3_cap_checkpoint_restore.c:160:clone3_cap_checkpoint_restore:Expected set_capability() (-1) == 0 (0)

What's the error number here? Looks like this test simply uses perror - it is better to use strerror() which includes the error number.

Is this related EPERM?

# clone3_cap_checkpoint_restore.c:161:clone3_cap_checkpoint_restore:Could not set CAP_CHECKPOINT_RESTORE # clone3_cap_checkpoint_restore: Test terminated by assertion # FAIL global.clone3_cap_checkpoint_restore

Changing to using capget and capset syscall directly here can fix this error, just like what the commit 663af70aabb7 ("bpf: selftests: Add helpers to directly use the capget and capset syscall") does.

Is this still accurate for v3 - Does this patch match the bpf commit?

What is the output with this patch? Include it in the change log.

Signed-off-by: zhouyuhang zhouyuhang@kylinos.cn> ---

Please mention the changes from v2 to v3 here so it makes it easier for reviewers associating the changes to the reviewer.

I had to go look up v1 and v2.

.../clone3/clone3_cap_checkpoint_restore.c | 58 +++++++++---------- 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c index 3c196fa86c99..8b61702bf721 100644 --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c @@ -27,6 +27,13 @@ #include "../kselftest_harness.h" #include "clone3_selftests.h" +/*

• • Prevent not being defined in the header file
• */

+#ifndef CAP_CHECKPOINT_RESTORE +#define CAP_CHECKPOINT_RESTORE 40 +#endif

• 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,
  

cap_validate_magic() handles _LINUX_CAPABILITY_VERSION_1, _LINUX_CAPABILITY_VERSION_2, and _LINUX_CAPABILITY_VERSION_3

It would help to add a comment on why it is necessary to set the version here.

• };
• __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID;
• __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32);

Explain why this is necessary - a comment will help future maintenance of this code.

• int ret;
• ret = capget(&hdr, data);
• if (ret) {

• perror("capget");
  

return -1;

} /* Drop all capabilities */

• if (cap_clear(caps)) {

• perror("cap_clear");
  

• goto out;
  

• }
• cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
• cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);

• memset(&data, 0, sizeof(data));

• cap = (struct libcap *) caps;

• data[0].effective |= cap0;
• data[0].permitted |= cap0;

• /* 40 -> CAP_CHECKPOINT_RESTORE */
• cap->data[1].effective |= 1 << (40 - 32);
• cap->data[1].permitted |= 1 << (40 - 32);

• data[1].effective |= cap1;
• data[1].permitted |= cap1;

• 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; }