Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add verifier test exercising jit PROBE_MEM logic

[Yonghong Song <yhs@meta.com>]

On 12/16/22 10:31 AM, Dave Marchevsky wrote:
> This patch adds a test exercising logic that was fixed / improved in
> the previous patch in the series, as well as general sanity checking for
> jit's PROBE_MEM logic which should've been unaffected by the previous
> patch.
> 
> The added verifier test does the following:
>    * Acquire a referenced kptr to struct prog_test_ref_kfunc using
>      existing net/bpf/test_run.c kfunc
>      * Helper returns ptr to a specific prog_test_ref_kfunc whose first
>        two fields - both ints - have been prepopulated w/ vals 42 and
>        108, respectively
>    * kptr_xchg the acquired ptr into an arraymap
>    * Do a direct map_value load of the just-added ptr
>      * Goal of all this setup is to get an unreferenced kptr pointing to
>        struct with ints of known value, which is the result of this step
>    * Using unreferenced kptr obtained in previous step, do loads of
>      prog_test_ref_kfunc.a (offset 0) and .b (offset 4)
>    * Then incr the kptr by 8 and load prog_test_ref_kfunc.a again (this
>      time at offset -8)
>    * Add all the loaded ints together and return
> 
> Before the PROBE_MEM fixes in previous patch, the loads at offset 0 and
> 4 would succeed, while the load at offset -8 would incorrectly fail
> runtime check emitted by the JIT and 0 out dst reg as a result. This
> confirmed by retval of 150 for this test before previous patch - since
> second .a read is 0'd out - and a retval of 192 with the fixed logic.
> 
> The test exercises the two optimizations to fixed logic added in last
> patch as well:
>    * First load, with insn "r8 = *(u32 *)(r9 + 0)" exercises "insn->off
>      is 0, no need to add / sub from src_reg" optimization
>    * Third load, with insn "r9 = *(u32 *)(r9 - 8)" exercises "src_reg ==
>      dst_reg, no need to restore src_reg after load" optimization
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Ack with one nit below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
> v1 -> v2: lore.kernel.org/bpf/20221213182726.325137-2-davemarchevsky@fb.com
>    * Rewrite the test to be a "normal" C prog in selftests/bpf/progs. Result
>      is a much easier-to-understand test with assembly used only for the 3
>      loads. (Yonghong)
> 
>   .../selftests/bpf/prog_tests/jit_probe_mem.c  | 28 +++++++++
>   .../selftests/bpf/progs/jit_probe_mem.c       | 61 +++++++++++++++++++
>   2 files changed, 89 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/jit_probe_mem.c
>   create mode 100644 tools/testing/selftests/bpf/progs/jit_probe_mem.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/jit_probe_mem.c b/tools/testing/selftests/bpf/prog_tests/jit_probe_mem.c
> new file mode 100644
> index 000000000000..5639428607e6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/jit_probe_mem.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +#include "jit_probe_mem.skel.h"
> +
> +void test_jit_probe_mem(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts,
> +		.data_in = &pkt_v4,
> +		.data_size_in = sizeof(pkt_v4),
> +		.repeat = 1,
> +	);
> +	struct jit_probe_mem *skel;
> +	int ret;
> +
> +	skel = jit_probe_mem__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "jit_probe_mem__open_and_load"))
> +		return;
> +
> +	ret = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.test_jit_probe_mem), &opts);
> +	ASSERT_OK(ret, "jit_probe_mem ret");
> +	ASSERT_OK(opts.retval, "jit_probe_mem opts.retval");
> +	ASSERT_EQ(skel->data->total_sum, 192, "jit_probe_mem total_sum");
> +
> +	jit_probe_mem__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/jit_probe_mem.c b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> new file mode 100644
> index 000000000000..3bb8af4df837
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_helpers.h>
> +
> +static struct prog_test_ref_kfunc __kptr_ref *v;
> +long total_sum = -1;
> +
> +extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> +
> +SEC("tc")
> +int test_jit_probe_mem(struct __sk_buff *ctx)
> +{
> +	struct prog_test_ref_kfunc *p;
> +	unsigned long zero = 0, sum;
> +
> +	p = bpf_kfunc_call_test_acquire(&zero);
> +	if (!p)
> +		return 1;
> +
> +	p = bpf_kptr_xchg(&v, p);
> +	if (p)
> +		goto release_out;
> +
> +	/* Direct map value access of kptr, should be PTR_UNTRUSTED */
> +	p = v;
> +	if (!p)
> +		return 1;
> +
> +	asm volatile (
> +		"r9 = %[p];\n"
> +		"%[sum] = 0;\n"
> +
> +		/* r8 = p->a */
> +		"r8 = *(u32 *)(r9 + 0);\n"
> +		"%[sum] += r8;\n"
> +
> +		/* r8 = p->b */
> +		"r8 = *(u32 *)(r9 + 4);\n"
> +		"%[sum] += r8;\n"
> +
> +		"r9 += 8;\n"
> +		/* r9 = p->a */
> +		"r9 = *(u32 *)(r9 - 8);\n"
> +		"%[sum] += r9;\n"

All these '\n' are not necessary.

> +
> +		: [sum] "=r"(sum)
> +		: [p] "r"(p)
> +		: "r8", "r9"
> +	);
> +
> +	total_sum = sum;
> +	return 0;
> +release_out:
> +	bpf_kfunc_call_test_release(p);
> +	return 1;
> +}
> +
> +char _license[] SEC("license") = "GPL";