diff mbox series

[v4,2/3] selftests: bpf: add bpf_cpumask_fill selftests

Message ID 20250305211235.368399-3-emil@etsalapatis.com (mailing list archive)
State Accepted
Commit 66be130f8bf9efac6aa0fc1145ddec93147a3549
Delegated to: BPF
Headers show
Series bpf: introduce helper for populating bpf_cpumask | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+0) this patch: 26 (+0)
netdev/cc_maintainers warning 12 maintainers not CCed: jolsa@kernel.org kpsingh@kernel.org linux-kselftest@vger.kernel.org shuah@kernel.org sdf@fomichev.me john.fastabend@gmail.com haoluo@google.com mykolal@fb.com houtao1@huawei.com thinker.li@gmail.com cupertino.miranda@oracle.com song@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning CHECK: From:/Signed-off-by: email comments mismatch: 'From: Emil Tsalapatis <emil@etsalapatis.com>' != 'Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>' CHECK: Please don't use multiple blank lines WARNING: externs should be avoided in .c files WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Emil Tsalapatis March 5, 2025, 9:12 p.m. UTC 
Add selftests for the bpf_cpumask_fill helper that sets a bpf_cpumask to
a bit pattern provided by a BPF program.

Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>
---
 .../selftests/bpf/progs/cpumask_failure.c     |  38 ++++++
 .../selftests/bpf/progs/cpumask_success.c     | 114 ++++++++++++++++++
 2 files changed, 152 insertions(+)

Comments

Hou Tao March 6, 2025, 1:56 a.m. UTC | #1 
Hi,

On 3/6/2025 5:12 AM, Emil Tsalapatis wrote:
> Add selftests for the bpf_cpumask_fill helper that sets a bpf_cpumask to
> a bit pattern provided by a BPF program.
>
> Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>
> ---
>  .../selftests/bpf/progs/cpumask_failure.c     |  38 ++++++
>  .../selftests/bpf/progs/cpumask_success.c     | 114 ++++++++++++++++++
>  2 files changed, 152 insertions(+)

My local build failed due to the missed declaration of
"bpf_cpumask_populate" in cpumask_common.h. It reported the following error:

progs/cpumask_success.c:788:8: error: call to undeclared function
'bpf_cpumask_populate'; ISO C99 and later do not support implicit fun
ction declarations [-Wimplicit-function-declaration]
  788 |         ret = bpf_cpumask_populate((struct cpumask *)local,
&toofewbits, sizeof(toofewbits));

Don't know the reason why CI succeeded.

> diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> index b40b52548ffb..8a2fd596c8a3 100644
> --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
> +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> @@ -222,3 +222,41 @@ int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flag
>  
>  	return 0;
>  }
> +
> +SEC("tp_btf/task_newtask")
> +__failure __msg("type=scalar expected=fp")
> +int BPF_PROG(test_populate_invalid_destination, struct task_struct *task, u64 clone_flags)
> +{
> +	struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
> +	u64 bits;
> +	int ret;
> +
> +	ret = bpf_cpumask_populate((struct cpumask *)invalid, &bits, sizeof(bits));
> +	if (!ret)
> +		err = 2;
> +
> +	return 0;
> +}
> +
> +SEC("tp_btf/task_newtask")
> +__failure __msg("leads to invalid memory access")
> +int BPF_PROG(test_populate_invalid_source, struct task_struct *task, u64 clone_flags)
> +{
> +	void *garbage = (void *)0x123456;
> +	struct bpf_cpumask *local;
> +	int ret;
> +
> +	local = create_cpumask();
> +	if (!local) {
> +		err = 1;
> +		return 0;
> +	}
> +
> +	ret = bpf_cpumask_populate((struct cpumask *)local, garbage, 8);
> +	if (!ret)
> +		err = 2;
> +
> +	bpf_cpumask_release(local);
> +
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
> index 80ee469b0b60..5dc0fe9940dc 100644
> --- a/tools/testing/selftests/bpf/progs/cpumask_success.c
> +++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
> @@ -757,6 +757,7 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
>  	mask1 = bpf_cpumask_create();
>  	mask2 = bpf_cpumask_create();
>  
> +
>  	if (!mask1 || !mask2)
>  		goto free_masks_return;

An extra newline.
>  
> @@ -770,3 +771,116 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
>  		bpf_cpumask_release(mask2);
>  	return 0;
>  }
> +
> +SEC("tp_btf/task_newtask")
> +__success

For tp_btf, bpf_prog_test_run() doesn't run the prog and it just returns
directly, therefore, the prog below is not exercised at all. How about
add test_populate_reject_small_mask into cpumask_success_testcases
firstly, then switch these test cases to use __success() in a following
patch ?
> +int BPF_PROG(test_populate_reject_small_mask, struct task_struct *task, u64 clone_flags)
> +{
> +	struct bpf_cpumask *local;
> +	u8 toofewbits;
> +	int ret;
> +
> +	local = create_cpumask();
> +	if (!local)
> +		return 0;
> +
> +	/* The kfunc should prevent this operation */
> +	ret = bpf_cpumask_populate((struct cpumask *)local, &toofewbits, sizeof(toofewbits));
> +	if (ret != -EACCES)
> +		err = 2;
> +
> +	bpf_cpumask_release(local);
> +
> +	return 0;
> +}
> +
> +/* Mask is guaranteed to be large enough for bpf_cpumask_t. */
> +#define CPUMASK_TEST_MASKLEN (sizeof(cpumask_t))
> +
> +/* Add an extra word for the test_populate_reject_unaligned test. */
> +u64 bits[CPUMASK_TEST_MASKLEN / 8 + 1];
> +extern bool CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS __kconfig __weak;
> +
> +SEC("tp_btf/task_newtask")
> +__success

Same for test_populate_reject_unaligned.
> +int BPF_PROG(test_populate_reject_unaligned, struct task_struct *task, u64 clone_flags)
> +{
> +	struct bpf_cpumask *mask;
> +	char *src;
> +	int ret;
> +
> +	/* Skip if unaligned accesses are fine for this arch.  */
> +	if (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> +		return 0;
> +
> +	mask = bpf_cpumask_create();
> +	if (!mask) {
> +		err = 1;
> +		return 0;
> +	}
> +
> +	/* Misalign the source array by a byte. */
> +	src = &((char *)bits)[1];
> +
> +	ret = bpf_cpumask_populate((struct cpumask *)mask, src, CPUMASK_TEST_MASKLEN);
> +	if (ret != -EINVAL)
> +		err = 2;
> +
> +	bpf_cpumask_release(mask);
> +
> +	return 0;
> +}
> +
> +
> +SEC("tp_btf/task_newtask")
> +__success
> +int BPF_PROG(test_populate, struct task_struct *task, u64 clone_flags)
> +{
> +	struct bpf_cpumask *mask;
> +	bool bit;
> +	int ret;
> +	int i;
> +
> +	/* Set only odd bits. */
> +	__builtin_memset(bits, 0xaa, CPUMASK_TEST_MASKLEN);
> +
> +	mask = bpf_cpumask_create();
> +	if (!mask) {
> +		err = 1;
> +		return 0;
> +	}
> +
> +	/* Pass the entire bits array, the kfunc will only copy the valid bits. */
> +	ret = bpf_cpumask_populate((struct cpumask *)mask, bits, CPUMASK_TEST_MASKLEN);
> +	if (ret) {
> +		err = 2;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Test is there to appease the verifier. We cannot directly
> +	 * access NR_CPUS, the upper bound for nr_cpus, so we infer
> +	 * it from the size of cpumask_t.
> +	 */
> +	if (nr_cpus < 0 || nr_cpus >= CPUMASK_TEST_MASKLEN * 8) {
> +		err = 3;
> +		goto out;
> +	}
> +
> +	bpf_for(i, 0, nr_cpus) {
> +		/* Odd-numbered bits should be set, even ones unset. */
> +		bit = bpf_cpumask_test_cpu(i, (const struct cpumask *)mask);
> +		if (bit == (i % 2 != 0))
> +			continue;
> +
> +		err = 4;
> +		break;
> +	}
> +
> +out:
> +	bpf_cpumask_release(mask);
> +
> +	return 0;
> +}
> +
> +#undef CPUMASK_TEST_MASKLEN
Alexei Starovoitov March 6, 2025, 2:20 a.m. UTC | #2 
On Wed, Mar 5, 2025 at 5:57 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 3/6/2025 5:12 AM, Emil Tsalapatis wrote:
> > Add selftests for the bpf_cpumask_fill helper that sets a bpf_cpumask to
> > a bit pattern provided by a BPF program.
> >
> > Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>
> > ---
> >  .../selftests/bpf/progs/cpumask_failure.c     |  38 ++++++
> >  .../selftests/bpf/progs/cpumask_success.c     | 114 ++++++++++++++++++
> >  2 files changed, 152 insertions(+)
>
> My local build failed due to the missed declaration of
> "bpf_cpumask_populate" in cpumask_common.h. It reported the following error:
>
> progs/cpumask_success.c:788:8: error: call to undeclared function
> 'bpf_cpumask_populate'; ISO C99 and later do not support implicit fun
> ction declarations [-Wimplicit-function-declaration]
>   788 |         ret = bpf_cpumask_populate((struct cpumask *)local,
> &toofewbits, sizeof(toofewbits));
>
> Don't know the reason why CI succeeded.

You need to upgrade pahole to make sure it emits kfuncs into vmlinux.h

>
> > diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > index b40b52548ffb..8a2fd596c8a3 100644
> > --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > @@ -222,3 +222,41 @@ int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flag
> >
> >       return 0;
> >  }
> > +
> > +SEC("tp_btf/task_newtask")
> > +__failure __msg("type=scalar expected=fp")
> > +int BPF_PROG(test_populate_invalid_destination, struct task_struct *task, u64 clone_flags)
> > +{
> > +     struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
> > +     u64 bits;
> > +     int ret;
> > +
> > +     ret = bpf_cpumask_populate((struct cpumask *)invalid, &bits, sizeof(bits));
> > +     if (!ret)
> > +             err = 2;
> > +
> > +     return 0;
> > +}
> > +
> > +SEC("tp_btf/task_newtask")
> > +__failure __msg("leads to invalid memory access")
> > +int BPF_PROG(test_populate_invalid_source, struct task_struct *task, u64 clone_flags)
> > +{
> > +     void *garbage = (void *)0x123456;
> > +     struct bpf_cpumask *local;
> > +     int ret;
> > +
> > +     local = create_cpumask();
> > +     if (!local) {
> > +             err = 1;
> > +             return 0;
> > +     }
> > +
> > +     ret = bpf_cpumask_populate((struct cpumask *)local, garbage, 8);
> > +     if (!ret)
> > +             err = 2;
> > +
> > +     bpf_cpumask_release(local);
> > +
> > +     return 0;
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > index 80ee469b0b60..5dc0fe9940dc 100644
> > --- a/tools/testing/selftests/bpf/progs/cpumask_success.c
> > +++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > @@ -757,6 +757,7 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
> >       mask1 = bpf_cpumask_create();
> >       mask2 = bpf_cpumask_create();
> >
> > +
> >       if (!mask1 || !mask2)
> >               goto free_masks_return;
>
> An extra newline.
> >
> > @@ -770,3 +771,116 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
> >               bpf_cpumask_release(mask2);
> >       return 0;
> >  }
> > +
> > +SEC("tp_btf/task_newtask")
> > +__success
>
> For tp_btf, bpf_prog_test_run() doesn't run the prog and it just returns
> directly, therefore, the prog below is not exercised at all. How about
> add test_populate_reject_small_mask into cpumask_success_testcases
> firstly, then switch these test cases to use __success() in a following
> patch ?

Good point. I'll revert and wait for respin.
Emil Tsalapatis March 6, 2025, 2:36 a.m. UTC | #3 
Hi,

thank you for the feedback. I will address it in a v5.

On Wed, Mar 5, 2025 at 8:57 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 3/6/2025 5:12 AM, Emil Tsalapatis wrote:
> > Add selftests for the bpf_cpumask_fill helper that sets a bpf_cpumask to
> > a bit pattern provided by a BPF program.
> >
> > Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>
> > ---
> >  .../selftests/bpf/progs/cpumask_failure.c     |  38 ++++++
> >  .../selftests/bpf/progs/cpumask_success.c     | 114 ++++++++++++++++++
> >  2 files changed, 152 insertions(+)
>
> My local build failed due to the missed declaration of
> "bpf_cpumask_populate" in cpumask_common.h. It reported the following error:
>
> progs/cpumask_success.c:788:8: error: call to undeclared function
> 'bpf_cpumask_populate'; ISO C99 and later do not support implicit fun
> ction declarations [-Wimplicit-function-declaration]
>   788 |         ret = bpf_cpumask_populate((struct cpumask *)local,
> &toofewbits, sizeof(toofewbits));
>
> Don't know the reason why CI succeeded.
>

Based on Alexei's email systems with recent pahole versions handle
this fine (at least the CI and my local setup),
I will still add the definition in cpumask_common.h for uniformity
since all the other kfuncs have one.

> > diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > index b40b52548ffb..8a2fd596c8a3 100644
> > --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
> > @@ -222,3 +222,41 @@ int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flag
> >
> >       return 0;
> >  }
> > +
> > +SEC("tp_btf/task_newtask")
> > +__failure __msg("type=scalar expected=fp")
> > +int BPF_PROG(test_populate_invalid_destination, struct task_struct *task, u64 clone_flags)
> > +{
> > +     struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
> > +     u64 bits;
> > +     int ret;
> > +
> > +     ret = bpf_cpumask_populate((struct cpumask *)invalid, &bits, sizeof(bits));
> > +     if (!ret)
> > +             err = 2;
> > +
> > +     return 0;
> > +}
> > +
> > +SEC("tp_btf/task_newtask")
> > +__failure __msg("leads to invalid memory access")
> > +int BPF_PROG(test_populate_invalid_source, struct task_struct *task, u64 clone_flags)
> > +{
> > +     void *garbage = (void *)0x123456;
> > +     struct bpf_cpumask *local;
> > +     int ret;
> > +
> > +     local = create_cpumask();
> > +     if (!local) {
> > +             err = 1;
> > +             return 0;
> > +     }
> > +
> > +     ret = bpf_cpumask_populate((struct cpumask *)local, garbage, 8);
> > +     if (!ret)
> > +             err = 2;
> > +
> > +     bpf_cpumask_release(local);
> > +
> > +     return 0;
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > index 80ee469b0b60..5dc0fe9940dc 100644
> > --- a/tools/testing/selftests/bpf/progs/cpumask_success.c
> > +++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > @@ -757,6 +757,7 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
> >       mask1 = bpf_cpumask_create();
> >       mask2 = bpf_cpumask_create();
> >
> > +
> >       if (!mask1 || !mask2)
> >               goto free_masks_return;
>
> An extra newline.
> >
> > @@ -770,3 +771,116 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
> >               bpf_cpumask_release(mask2);
> >       return 0;
> >  }
> > +
> > +SEC("tp_btf/task_newtask")
> > +__success
>
> For tp_btf, bpf_prog_test_run() doesn't run the prog and it just returns
> directly, therefore, the prog below is not exercised at all. How about
> add test_populate_reject_small_mask into cpumask_success_testcases
> firstly, then switch these test cases to use __success() in a following
> patch ?

Sorry about that, I had the selftests properly hooked into
prog_tests/cpumask.c until v3 but saw duplicate entries in the
selftest log
and thought it was being run twice. I will add them back in.

Is __success() a different annotation? AFAICT __success is enough as
long as err is set to nonzero on an error path, and all
error paths are set like that in the selftests. In that case,
shouldn't adding the new tests cpumask_success_testcases be
enough to properly run the tests?


> > +int BPF_PROG(test_populate_reject_small_mask, struct task_struct *task, u64 clone_flags)
> > +{
> > +     struct bpf_cpumask *local;
> > +     u8 toofewbits;
> > +     int ret;
> > +
> > +     local = create_cpumask();
> > +     if (!local)
> > +             return 0;
> > +
> > +     /* The kfunc should prevent this operation */
> > +     ret = bpf_cpumask_populate((struct cpumask *)local, &toofewbits, sizeof(toofewbits));
> > +     if (ret != -EACCES)
> > +             err = 2;
> > +
> > +     bpf_cpumask_release(local);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Mask is guaranteed to be large enough for bpf_cpumask_t. */
> > +#define CPUMASK_TEST_MASKLEN (sizeof(cpumask_t))
> > +
> > +/* Add an extra word for the test_populate_reject_unaligned test. */
> > +u64 bits[CPUMASK_TEST_MASKLEN / 8 + 1];
> > +extern bool CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS __kconfig __weak;
> > +
> > +SEC("tp_btf/task_newtask")
> > +__success
>
> Same for test_populate_reject_unaligned.
> > +int BPF_PROG(test_populate_reject_unaligned, struct task_struct *task, u64 clone_flags)
> > +{
> > +     struct bpf_cpumask *mask;
> > +     char *src;
> > +     int ret;
> > +
> > +     /* Skip if unaligned accesses are fine for this arch.  */
> > +     if (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> > +             return 0;
> > +
> > +     mask = bpf_cpumask_create();
> > +     if (!mask) {
> > +             err = 1;
> > +             return 0;
> > +     }
> > +
> > +     /* Misalign the source array by a byte. */
> > +     src = &((char *)bits)[1];
> > +
> > +     ret = bpf_cpumask_populate((struct cpumask *)mask, src, CPUMASK_TEST_MASKLEN);
> > +     if (ret != -EINVAL)
> > +             err = 2;
> > +
> > +     bpf_cpumask_release(mask);
> > +
> > +     return 0;
> > +}
> > +
> > +
> > +SEC("tp_btf/task_newtask")
> > +__success
> > +int BPF_PROG(test_populate, struct task_struct *task, u64 clone_flags)
> > +{
> > +     struct bpf_cpumask *mask;
> > +     bool bit;
> > +     int ret;
> > +     int i;
> > +
> > +     /* Set only odd bits. */
> > +     __builtin_memset(bits, 0xaa, CPUMASK_TEST_MASKLEN);
> > +
> > +     mask = bpf_cpumask_create();
> > +     if (!mask) {
> > +             err = 1;
> > +             return 0;
> > +     }
> > +
> > +     /* Pass the entire bits array, the kfunc will only copy the valid bits. */
> > +     ret = bpf_cpumask_populate((struct cpumask *)mask, bits, CPUMASK_TEST_MASKLEN);
> > +     if (ret) {
> > +             err = 2;
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Test is there to appease the verifier. We cannot directly
> > +      * access NR_CPUS, the upper bound for nr_cpus, so we infer
> > +      * it from the size of cpumask_t.
> > +      */
> > +     if (nr_cpus < 0 || nr_cpus >= CPUMASK_TEST_MASKLEN * 8) {
> > +             err = 3;
> > +             goto out;
> > +     }
> > +
> > +     bpf_for(i, 0, nr_cpus) {
> > +             /* Odd-numbered bits should be set, even ones unset. */
> > +             bit = bpf_cpumask_test_cpu(i, (const struct cpumask *)mask);
> > +             if (bit == (i % 2 != 0))
> > +                     continue;
> > +
> > +             err = 4;
> > +             break;
> > +     }
> > +
> > +out:
> > +     bpf_cpumask_release(mask);
> > +
> > +     return 0;
> > +}
> > +
> > +#undef CPUMASK_TEST_MASKLEN
>
Hou Tao March 6, 2025, 6:59 a.m. UTC | #4 
Hi,

On 3/6/2025 10:36 AM, Emil Tsalapatis wrote:
> Hi,
>
> thank you for the feedback. I will address it in a v5.
>
> On Wed, Mar 5, 2025 at 8:57 PM Hou Tao <houtao@huaweicloud.com> wrote:
>> Hi,
>>
>> On 3/6/2025 5:12 AM, Emil Tsalapatis wrote:
>>> Add selftests for the bpf_cpumask_fill helper that sets a bpf_cpumask to
>>> a bit pattern provided by a BPF program.

Just find out, the name of bpf_cpumask_fill() also needs update.
>>>
>>> Signed-off-by: Emil Tsalapatis (Meta) <emil@etsalapatis.com>
>>> ---
>>>  .../selftests/bpf/progs/cpumask_failure.c     |  38 ++++++
>>>  .../selftests/bpf/progs/cpumask_success.c     | 114 ++++++++++++++++++
>>>  2 files changed, 152 insertions(+)
>> My local build failed due to the missed declaration of
>> "bpf_cpumask_populate" in cpumask_common.h. It reported the following error:
>>
>> progs/cpumask_success.c:788:8: error: call to undeclared function
>> 'bpf_cpumask_populate'; ISO C99 and later do not support implicit fun
>> ction declarations [-Wimplicit-function-declaration]
>>   788 |         ret = bpf_cpumask_populate((struct cpumask *)local,
>> &toofewbits, sizeof(toofewbits));
>>
>> Don't know the reason why CI succeeded.
>>
> Based on Alexei's email systems with recent pahole versions handle
> this fine (at least the CI and my local setup),
> I will still add the definition in cpumask_common.h for uniformity
> since all the other kfuncs have one.

I see. Thanks for that.
>
>>> diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
>>> index b40b52548ffb..8a2fd596c8a3 100644
>>> --- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
>>> +++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
>>> @@ -222,3 +222,41 @@ int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flag
>>>
>>>       return 0;
>>>  }
>>> +
>>> +SEC("tp_btf/task_newtask")
>>> +__failure __msg("type=scalar expected=fp")
>>> +int BPF_PROG(test_populate_invalid_destination, struct task_struct *task, u64 clone_flags)
>>> +{
>>> +     struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
>>> +     u64 bits;
>>> +     int ret;
>>> +
>>> +     ret = bpf_cpumask_populate((struct cpumask *)invalid, &bits, sizeof(bits));
>>> +     if (!ret)
>>> +             err = 2;
>>> +
>>> +     return 0;
>>> +}
>>> +

SNIP
>> An extra newline.
>>> @@ -770,3 +771,116 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
>>>               bpf_cpumask_release(mask2);
>>>       return 0;
>>>  }
>>> +
>>> +SEC("tp_btf/task_newtask")
>>> +__success
>> For tp_btf, bpf_prog_test_run() doesn't run the prog and it just returns
>> directly, therefore, the prog below is not exercised at all. How about
>> add test_populate_reject_small_mask into cpumask_success_testcases
>> firstly, then switch these test cases to use __success() in a following
>> patch ?
> Sorry about that, I had the selftests properly hooked into
> prog_tests/cpumask.c until v3 but saw duplicate entries in the
> selftest log
> and thought it was being run twice. I will add them back in.
>
> Is __success() a different annotation? AFAICT __success is enough as
> long as err is set to nonzero on an error path, and all
> error paths are set like that in the selftests. In that case,
> shouldn't adding the new tests cpumask_success_testcases be
> enough to properly run the tests?

Yes. __success() annotation is a bit different. It uses
bpf_prog_test_run() to run the bpf prog directly instead of trigger the
running of prog through an external event. I think adding new tests in
cpumask_success_testcases will be enough. However, there is one success
test test_refcount_null_tracking in cpumask_success.c which uses
__success annotation, and it is still buggy. I think it would be better
to switch all test cases to use __success annotation because the
annotation provides much clarity.
>
>
>>> +int BPF_PROG(test_populate_reject_small_mask, struct task_struct *task, u64 clone_flags)
>>> +{
>>> +     struct bpf_cpumask *local;
>>> +     u8 toofewbits;
>>> +     int ret;
>>> +
>>> +     local = create_cpumask();
>>> +     if (!local)
>>> +             return 0;
>>> +
>>> +     /* The kfunc should prevent this operation */
>>> +     ret = bpf_cpumask_populate((struct cpumask *)local, &toofewbits, sizeof(toofewbits));
>>> +     if (ret != -EACCES)
>>> +             err = 2;
>>> +
>>> +     bpf_cpumask_release(local);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/* Mask is guaranteed to be large enough for bpf_cpumask_t. */
>>> +#define CPUMASK_TEST_MASKLEN (sizeof(cpumask_t))
>>> +
>>> +/* Add an extra word for the test_populate_reject_unaligned test. */
>>> +u64 bits[CPUMASK_TEST_MASKLEN / 8 + 1];
>>> +extern bool CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS __kconfig __weak;
>>> +
>>> +SEC("tp_btf/task_newtask")
>>> +__success
>> Same for test_populate_reject_unaligned.
>>> +int BPF_PROG(test_populate_reject_unaligned, struct task_struct *task, u64 clone_flags)
>>> +{
>>> +     struct bpf_cpumask *mask;
>>> +     char *src;
>>> +     int ret;
>>> +
>>> +     /* Skip if unaligned accesses are fine for this arch.  */
>>> +     if (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>>> +             return 0;
>>> +
>>> +     mask = bpf_cpumask_create();
>>> +     if (!mask) {
>>> +             err = 1;
>>> +             return 0;
>>> +     }
>>> +
>>> +     /* Misalign the source array by a byte. */
>>> +     src = &((char *)bits)[1];
>>> +
>>> +     ret = bpf_cpumask_populate((struct cpumask *)mask, src, CPUMASK_TEST_MASKLEN);
>>> +     if (ret != -EINVAL)
>>> +             err = 2;
>>> +
>>> +     bpf_cpumask_release(mask);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +
>>> +SEC("tp_btf/task_newtask")
>>> +__success
>>> +int BPF_PROG(test_populate, struct task_struct *task, u64 clone_flags)
>>> +{
>>> +     struct bpf_cpumask *mask;
>>> +     bool bit;
>>> +     int ret;
>>> +     int i;
>>> +
>>> +     /* Set only odd bits. */
>>> +     __builtin_memset(bits, 0xaa, CPUMASK_TEST_MASKLEN);
>>> +
>>> +     mask = bpf_cpumask_create();
>>> +     if (!mask) {
>>> +             err = 1;
>>> +             return 0;
>>> +     }
>>> +
>>> +     /* Pass the entire bits array, the kfunc will only copy the valid bits. */
>>> +     ret = bpf_cpumask_populate((struct cpumask *)mask, bits, CPUMASK_TEST_MASKLEN);
>>> +     if (ret) {
>>> +             err = 2;
>>> +             goto out;
>>> +     }
>>> +
>>> +     /*
>>> +      * Test is there to appease the verifier. We cannot directly
>>> +      * access NR_CPUS, the upper bound for nr_cpus, so we infer
>>> +      * it from the size of cpumask_t.
>>> +      */
>>> +     if (nr_cpus < 0 || nr_cpus >= CPUMASK_TEST_MASKLEN * 8) {
>>> +             err = 3;
>>> +             goto out;
>>> +     }
>>> +
>>> +     bpf_for(i, 0, nr_cpus) {
>>> +             /* Odd-numbered bits should be set, even ones unset. */
>>> +             bit = bpf_cpumask_test_cpu(i, (const struct cpumask *)mask);
>>> +             if (bit == (i % 2 != 0))
>>> +                     continue;
>>> +
>>> +             err = 4;
>>> +             break;
>>> +     }
>>> +
>>> +out:
>>> +     bpf_cpumask_release(mask);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +#undef CPUMASK_TEST_MASKLEN
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index b40b52548ffb..8a2fd596c8a3 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -222,3 +222,41 @@  int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flag
 
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__failure __msg("type=scalar expected=fp")
+int BPF_PROG(test_populate_invalid_destination, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
+	u64 bits;
+	int ret;
+
+	ret = bpf_cpumask_populate((struct cpumask *)invalid, &bits, sizeof(bits));
+	if (!ret)
+		err = 2;
+
+	return 0;
+}
+
+SEC("tp_btf/task_newtask")
+__failure __msg("leads to invalid memory access")
+int BPF_PROG(test_populate_invalid_source, struct task_struct *task, u64 clone_flags)
+{
+	void *garbage = (void *)0x123456;
+	struct bpf_cpumask *local;
+	int ret;
+
+	local = create_cpumask();
+	if (!local) {
+		err = 1;
+		return 0;
+	}
+
+	ret = bpf_cpumask_populate((struct cpumask *)local, garbage, 8);
+	if (!ret)
+		err = 2;
+
+	bpf_cpumask_release(local);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index 80ee469b0b60..5dc0fe9940dc 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -757,6 +757,7 @@  int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
 	mask1 = bpf_cpumask_create();
 	mask2 = bpf_cpumask_create();
 
+
 	if (!mask1 || !mask2)
 		goto free_masks_return;
 
@@ -770,3 +771,116 @@  int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
 		bpf_cpumask_release(mask2);
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__success
+int BPF_PROG(test_populate_reject_small_mask, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local;
+	u8 toofewbits;
+	int ret;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	/* The kfunc should prevent this operation */
+	ret = bpf_cpumask_populate((struct cpumask *)local, &toofewbits, sizeof(toofewbits));
+	if (ret != -EACCES)
+		err = 2;
+
+	bpf_cpumask_release(local);
+
+	return 0;
+}
+
+/* Mask is guaranteed to be large enough for bpf_cpumask_t. */
+#define CPUMASK_TEST_MASKLEN (sizeof(cpumask_t))
+
+/* Add an extra word for the test_populate_reject_unaligned test. */
+u64 bits[CPUMASK_TEST_MASKLEN / 8 + 1];
+extern bool CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS __kconfig __weak;
+
+SEC("tp_btf/task_newtask")
+__success
+int BPF_PROG(test_populate_reject_unaligned, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *mask;
+	char *src;
+	int ret;
+
+	/* Skip if unaligned accesses are fine for this arch.  */
+	if (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+		return 0;
+
+	mask = bpf_cpumask_create();
+	if (!mask) {
+		err = 1;
+		return 0;
+	}
+
+	/* Misalign the source array by a byte. */
+	src = &((char *)bits)[1];
+
+	ret = bpf_cpumask_populate((struct cpumask *)mask, src, CPUMASK_TEST_MASKLEN);
+	if (ret != -EINVAL)
+		err = 2;
+
+	bpf_cpumask_release(mask);
+
+	return 0;
+}
+
+
+SEC("tp_btf/task_newtask")
+__success
+int BPF_PROG(test_populate, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *mask;
+	bool bit;
+	int ret;
+	int i;
+
+	/* Set only odd bits. */
+	__builtin_memset(bits, 0xaa, CPUMASK_TEST_MASKLEN);
+
+	mask = bpf_cpumask_create();
+	if (!mask) {
+		err = 1;
+		return 0;
+	}
+
+	/* Pass the entire bits array, the kfunc will only copy the valid bits. */
+	ret = bpf_cpumask_populate((struct cpumask *)mask, bits, CPUMASK_TEST_MASKLEN);
+	if (ret) {
+		err = 2;
+		goto out;
+	}
+
+	/*
+	 * Test is there to appease the verifier. We cannot directly
+	 * access NR_CPUS, the upper bound for nr_cpus, so we infer
+	 * it from the size of cpumask_t.
+	 */
+	if (nr_cpus < 0 || nr_cpus >= CPUMASK_TEST_MASKLEN * 8) {
+		err = 3;
+		goto out;
+	}
+
+	bpf_for(i, 0, nr_cpus) {
+		/* Odd-numbered bits should be set, even ones unset. */
+		bit = bpf_cpumask_test_cpu(i, (const struct cpumask *)mask);
+		if (bit == (i % 2 != 0))
+			continue;
+
+		err = 4;
+		break;
+	}
+
+out:
+	bpf_cpumask_release(mask);
+
+	return 0;
+}
+
+#undef CPUMASK_TEST_MASKLEN