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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.

> 
> 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.

> +	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.

> +
>   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?

> +
> +#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.

> +
> +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,
> +};
> +