Re: [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/7/24 14:38, Martin KaFai Lau wrote:
> On 2/5/24 10:38 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Test if the verifier verifies nullable pointer arguments correctly for 
>> BPF
>> struct_ops programs.
>>
>> "test_maybe_null" in struct bpf_testmod_ops is the operator defined 
>> for the
>> test cases here. It has several pointer arguments to various types. These
>> pointers are majorly classified to 3 categories; pointers to struct 
>> types,
>> pointers to scalar types, and pointers to array types. They are handled
>> sightly differently.
> 
> The commit message needs an update. probably make sense to skip what 
> pointer type is supported because this patch set does not change that.

Agree!

> 
>>
>> A BPF program should check a pointer for NULL beforehand to access the
>> value pointed by the nullable pointer arguments, or the verifier should
>> reject the programs. The test here includes two parts; the programs
>> checking pointers properly and the programs not checking pointers
>> beforehand. The test checks if the verifier accepts the programs checking
>> properly and rejects the programs not checking at all.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 12 ++++-
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  7 +++
>>   .../prog_tests/test_struct_ops_maybe_null.c   | 47 +++++++++++++++++++
>>   .../bpf/progs/struct_ops_maybe_null.c         | 31 ++++++++++++
>>   .../bpf/progs/struct_ops_maybe_null_fail.c    | 25 ++++++++++
>>   5 files changed, 121 insertions(+), 1 deletion(-)
>>   create mode 100644 
>> tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
>>   create mode 100644 
>> tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
>>   create mode 100644 
>> tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c 
>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index a06daebc75c9..891a2b5f422c 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -555,7 +555,10 @@ static int bpf_dummy_reg(void *kdata)
>>   {
>>       struct bpf_testmod_ops *ops = kdata;
>> -    ops->test_2(4, 3);
>> +    if (ops->test_maybe_null)
>> +        ops->test_maybe_null(0, NULL);
> 
> afaict, the "static void maybe_null(void)" test below does not exercise 
> this line of change.

I will remove it.

> 
>> +    else
>> +        ops->test_2(4, 3);
>>       return 0;
>>   }
>> @@ -573,9 +576,16 @@ static void bpf_testmod_test_2(int a, int b)
>>   {
>>   }
>> +static int bpf_testmod_ops__test_maybe_null(int dummy,
>> +                        struct task_struct *task__nullable)
>> +{
>> +    return 0;
>> +}
>> +
>>   static struct bpf_testmod_ops __bpf_testmod_ops = {
>>       .test_1 = bpf_testmod_test_1,
>>       .test_2 = bpf_testmod_test_2,
>> +    .test_maybe_null = bpf_testmod_ops__test_maybe_null,
>>   };
>>   struct bpf_struct_ops bpf_bpf_testmod_ops = {
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h 
>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
>> index 537beca42896..c51580c9119d 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
>> @@ -5,6 +5,8 @@
>>   #include <linux/types.h>
>> +struct task_struct;
>> +
>>   struct bpf_testmod_test_read_ctx {
>>       char *buf;
>>       loff_t off;
>> @@ -28,9 +30,14 @@ struct bpf_iter_testmod_seq {
>>       int cnt;
>>   };
>> +typedef u32 (*ar_t)[2];
>> +typedef u32 (*ar2_t)[];
> 
> They are not needed in v5.

Sure!

> 
>> +
>>   struct bpf_testmod_ops {
>>       int (*test_1)(void);
>>       void (*test_2)(int a, int b);
>> +    /* Used to test nullable arguments. */
>> +    int (*test_maybe_null)(int dummy, struct task_struct *task);
>>   };
>>   #endif /* _BPF_TESTMOD_H */
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c 
>> b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
>> new file mode 100644
>> index 000000000000..1c057c62d893
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +#include <test_progs.h>
>> +#include <time.h>
> 
> Why time.h?

It should be removed now.

> 
>> +
>> +#include "struct_ops_maybe_null.skel.h"
>> +#include "struct_ops_maybe_null_fail.skel.h"
>> +
>> +/* Test that the verifier accepts a program that access a nullable 
>> pointer
>> + * with a proper check.
>> + */
>> +static void maybe_null(void)
>> +{
>> +    struct struct_ops_maybe_null *skel;
>> +
>> +    skel = struct_ops_maybe_null__open_and_load();
>> +    if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_and_load"))
>> +        return;
>> +
>> +    struct_ops_maybe_null__destroy(skel);
>> +}
>> +
>> +/* Test that the verifier rejects a program that access a nullable 
>> pointer
>> + * without a check beforehand.
>> + */
>> +static void maybe_null_fail(void)
>> +{
>> +    struct struct_ops_maybe_null_fail *skel;
>> +
>> +    skel = struct_ops_maybe_null_fail__open_and_load();
>> +    if (ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
>> +        return;
>> +
>> +    struct_ops_maybe_null_fail__destroy(skel);
>> +}
>> +
>> +void test_struct_ops_maybe_null(void)
>> +{
>> +    /* The verifier verifies the programs at load time, so testing both
>> +     * programs in the same compile-unit is complicated. We run them in
>> +     * separate objects to simplify the testing.
>> +     */
>> +    if (test__start_subtest("maybe_null"))
>> +        maybe_null();
>> +    if (test__start_subtest("maybe_null_fail"))
>> +        maybe_null_fail();
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c 
>> b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
>> new file mode 100644
>> index 000000000000..c5769c742900
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +u64 tgid = 0;
> 
> u64 here.
> 
>> +
>> +/* This is a test BPF program that uses struct_ops to access an argument
>> + * that may be NULL. This is a test for the verifier to ensure that 
>> it can
>> + * rip PTR_MAYBE_NULL correctly. There are tree pointers; task, 
>> scalar, and
>> + * ar. They are used to test the cases of PTR_TO_BTF_ID, PTR_TO_BUF, 
>> and array.
>> + */
>> +SEC("struct_ops/test_maybe_null")
>> +int BPF_PROG(test_maybe_null, int dummy,
>> +         struct task_struct *task)
>> +{
>> +    if (task)
>> +        tgid = task->tgid;
>> +
>> +    return 0;
>> +}
>> +
>> +SEC(".struct_ops.link")
>> +struct bpf_testmod_ops testmod_1 = {
>> +    .test_maybe_null = (void *)test_maybe_null,
>> +};
>> +
>> diff --git 
>> a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c 
>> b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
>> new file mode 100644
>> index 000000000000..566be47fb40b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +int tgid = 0;
> 
> but int here.
> 
> understand that it does not matter and not the focus of this test but 
> still better be consistent and use the correct one.

I will chnage them to pid_t.

> 
>> +
>> +SEC("struct_ops/test_maybe_null_struct_ptr")
>> +int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
>> +         struct task_struct *task)
>> +{
>> +    tgid = task->tgid;
>> +
>> +    return 0;
>> +}
>> +
>> +SEC(".struct_ops.link")
>> +struct bpf_testmod_ops testmod_struct_ptr = {
>> +    .test_maybe_null = (void *)test_maybe_null_struct_ptr,
>> +};
>> +
>