Hi,
On Fri, Feb 28, 2025 at 8:16 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 2/28/2025 8:33 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@xxxxxxxxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/verifier.c | 2 +
> > .../selftests/bpf/progs/cpumask_success.c | 23 ++++++
> > .../selftests/bpf/progs/verifier_cpumask.c | 77 +++++++++++++++++++
> > 3 files changed, 102 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/verifier_cpumask.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > index 8a0e1ff8a2dc..4dd95e93bd7e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > @@ -23,6 +23,7 @@
> > #include "verifier_cgroup_storage.skel.h"
> > #include "verifier_const.skel.h"
> > #include "verifier_const_or.skel.h"
> > +#include "verifier_cpumask.skel.h"
> > #include "verifier_ctx.skel.h"
> > #include "verifier_ctx_sk_msg.skel.h"
> > #include "verifier_d_path.skel.h"
> > @@ -155,6 +156,7 @@ void test_verifier_cgroup_skb(void) { RUN(verifier_cgroup_skb); }
> > void test_verifier_cgroup_storage(void) { RUN(verifier_cgroup_storage); }
> > void test_verifier_const(void) { RUN(verifier_const); }
> > void test_verifier_const_or(void) { RUN(verifier_const_or); }
> > +void test_verifier_cpumask(void) { RUN(verifier_cpumask); }
>
> Why is a new file necessary ? Is it more reasonable to add these success
> and failure test cases in cpumask_success.c and cpumask_failure.c ?
Sounds good, I will roll the new tests into the existing files.
> > void test_verifier_ctx(void) { RUN(verifier_ctx); }
> > void test_verifier_ctx_sk_msg(void) { RUN(verifier_ctx_sk_msg); }
> > void test_verifier_d_path(void) { RUN(verifier_d_path); }
> > diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > index 80ee469b0b60..f252aa2f3090 100644
> > --- a/tools/testing/selftests/bpf/progs/cpumask_success.c
> > +++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
> > @@ -770,3 +770,26 @@ int BPF_PROG(test_refcount_null_tracking, struct task_struct *task, u64 clone_fl
> > bpf_cpumask_release(mask2);
> > return 0;
> > }
> > +
> > +SEC("syscall")
> > +__success
> > +int BPF_PROG(test_fill_reject_small_mask)
> > +{
> > + struct bpf_cpumask *local;
> > + u8 toofewbits;
> > + int ret;
> > +
> > + local = create_cpumask();
> > + if (!local)
> > + return 0;
> > +
> > + /* The kfunc should prevent this operation */
> > + ret = bpf_cpumask_fill((struct cpumask *)local, &toofewbits, sizeof(toofewbits));
> > + if (ret != -EACCES)
> > + err = 2;
>
> The check may not be true when running local with a smaller NR_CPUS. It
> will be more reasonable to adjust the size according to the value of
> nr_cpu_ids.
Now that the size check rounds the size up to the nearest sizeof(long)
bytes, passing a
mask of size 1 is guaranteed to fail.
> > +
> > + bpf_cpumask_release(local);
> > +
> > + return 0;
> > +}
> > +
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_cpumask.c b/tools/testing/selftests/bpf/progs/verifier_cpumask.c
> > new file mode 100644
> > index 000000000000..bb84dd36beac
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/verifier_cpumask.c
> > @@ -0,0 +1,77 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_misc.h"
> > +
> > +#include "cpumask_common.h"
> > +
> > +#define CPUMASK_TEST_MASKLEN (8 * sizeof(u64))
> > +
> > +u64 bits[CPUMASK_TEST_MASKLEN];
> > +
> > +SEC("syscall")
> > +__success
> > +int BPF_PROG(test_cpumask_fill)
> > +{
> > + struct bpf_cpumask *mask;
> > + int ret;
> > +
> > + mask = bpf_cpumask_create();
> > + if (!mask) {
> > + err = 1;
> > + return 0;
> > + }
> > +
> > + ret = bpf_cpumask_fill((struct cpumask *)mask, bits, CPUMASK_TEST_MASKLEN);
> > + if (!ret)
> > + err = 2;
>
> It would be better to also test the cpu bits in the cpumask after
> bpf_cpumask_fill() is expected.
Sounds good, will address it.
> > +
> > + if (mask)
> > + bpf_cpumask_release(mask);
>
> The "if (mask)" check is unnecessary.
Ditto.
> > +
> > + return 0;
> > +}
> > +
> > +SEC("syscall")
> > +__description("bpf_cpumask_fill: invalid cpumask target")
> > +__failure __msg("type=scalar expected=fp")
> > +int BPF_PROG(test_cpumask_fill_cpumask_invalid)
> > +{
> > + struct bpf_cpumask *invalid = (struct bpf_cpumask *)0x123456;
> > + int ret;
> > +
> > + ret = bpf_cpumask_fill((struct cpumask *)invalid, bits, CPUMASK_TEST_MASKLEN);
> > + if (!ret)
> > + err = 2;
> > +
> > + return 0;
> > +}
> > +
> > +SEC("syscall")
> > +__description("bpf_cpumask_fill: invalid cpumask source")
> > +__failure __msg("leads to invalid memory access")
> > +int BPF_PROG(test_cpumask_fill_bpf_invalid)
> > +{
> > + void *garbage = (void *)0x123456;
> > + struct bpf_cpumask *local;
> > + int ret;
> > +
> > + local = create_cpumask();
> > + if (!local) {
> > + err = 1;
> > + return 0;
> > + }
> > +
> > + ret = bpf_cpumask_fill((struct cpumask *)local, garbage, CPUMASK_TEST_MASKLEN);
> > + if (!ret)
> > + err = 2;
> > +
> > + bpf_cpumask_release(local);
> > +
> > + return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
>
