Re: [PATCH bpf-next 3/3] selftests/bpf: Add unit tests for global functions

[Yonghong Song <yhs@fb.com>]

On 12/14/20 11:52 AM, Dmitrii Banshchikov wrote:
> test_global_func9  - check valid scenarios for struct pointers
> test_global_func10 - check that the smaller struct cannot be passed as a
>                       the larger one
> test_global_func11 - check that CTX pointer cannot be passed as a struct
>                       pointer
> test_global_func12 - check access to a null pointer
> test_global_func13 - check access to an arbitrary pointer value
> 
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>   .../bpf/prog_tests/test_global_funcs.c        |  5 ++
>   .../selftests/bpf/progs/test_global_func10.c  | 29 +++++++++
>   .../selftests/bpf/progs/test_global_func11.c  | 19 ++++++
>   .../selftests/bpf/progs/test_global_func12.c  | 21 +++++++
>   .../selftests/bpf/progs/test_global_func13.c  | 24 ++++++++
>   .../selftests/bpf/progs/test_global_func9.c   | 59 +++++++++++++++++++
>   6 files changed, 157 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> index 32e4348b714b..c4895e6c83c2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> @@ -61,6 +61,11 @@ void test_test_global_funcs(void)
>   		{ "test_global_func6.o" , "modified ctx ptr R2" },
>   		{ "test_global_func7.o" , "foo() doesn't return scalar" },
>   		{ "test_global_func8.o" },
> +		{ "test_global_func9.o" },
> +		{ "test_global_func10.o", "invalid indirect read from stack off -8+4 size 8" },
> +		{ "test_global_func11.o", "Caller passes invalid args into func#1" },
> +		{ "test_global_func12.o", "invalid mem access 'mem_or_null'" },
> +		{ "test_global_func13.o", "Caller passes invalid args into func#1" },
>   	};
>   	libbpf_print_fn_t old_print_fn = NULL;
>   	int err, i, duration = 0;
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func10.c b/tools/testing/selftests/bpf/progs/test_global_func10.c
> new file mode 100644
> index 000000000000..61c2ae92ce41
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func10.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct Small {
> +	int x;
> +};
> +
> +struct Big {
> +	int x;
> +	int y;
> +};
> +
> +__noinline int foo(const struct Big *big)
> +{
> +	if (big == 0)
> +		return 0;
> +
> +	return bpf_get_prandom_u32() < big->y;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	const struct Small small = {.x = skb->len };
> +
> +	return foo((struct Big *)&small) ? 1 : 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func11.c b/tools/testing/selftests/bpf/progs/test_global_func11.c
> new file mode 100644
> index 000000000000..28488047c849
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func11.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct S {
> +	int x;
> +};
> +
> +__noinline int foo(const struct S *s)
> +{
> +	return s ? bpf_get_prandom_u32() < s->x : 0;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	return foo(skb);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func12.c b/tools/testing/selftests/bpf/progs/test_global_func12.c
> new file mode 100644
> index 000000000000..62343527cc59
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func12.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct S {
> +	int x;
> +};
> +
> +__noinline int foo(const struct S *s)
> +{
> +	return bpf_get_prandom_u32() < s->x;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	const struct S s = {.x = skb->len };
> +
> +	return foo(&s);
> +}

I assume struct member write is also supported? In the next revision, 
could you also test case something like

struct S { int x; int y; };
__noinline int foo(struct S *s) {
     s->x = 1;
     return s->y;
};
SEC(...) int test(struct __sk_buff *skb) {
     struct S s = {.y = skb->len};
     int ret = foo(&s);
     return ret < 100 ? 0 : s.x;
}

In the above, to facilitate implementation, if initializing s.x is 
desirable, we can do
SEC(...) int test(struct __sk_buff *skb) {
     struct S s = {.x = 0, .y = skb->len};
     int ret = foo(&s);
     return ret < 100 ? 0 : s.x;
}

> diff --git a/tools/testing/selftests/bpf/progs/test_global_func13.c b/tools/testing/selftests/bpf/progs/test_global_func13.c
> new file mode 100644
> index 000000000000..ff8897c1ac22
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func13.c
[...]