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

[Shuah Khan <skhan@linuxfoundation.org>]

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");
>   		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