Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add test verifying bpf_ringbuf_reserve retval use in map ops

[Yonghong Song <yhs@fb.com>]

On 9/14/22 5:36 AM, Dave Marchevsky wrote:
> Add a test_ringbuf_map_key test prog, borrowing heavily from extant
> test_ringbuf.c. The program tries to use the result of
> bpf_ringbuf_reserve as map_key, which was not possible before previouis
> commits in this series. The test runner added to prog_tests/ringbuf.c
> verifies that the program loads and does basic sanity checks to confirm
> that it runs as expected.
> 
> Also, refactor test_ringbuf such that runners for existing test_ringbuf
> and newly-added test_ringbuf_map_key are subtests of 'ringbuf' top-level
> test.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@fb.com
> 
> * Actually run the program instead of just loading (Yonghong)
> * Add a bpf_map_update_elem call to the test (Yonghong)
> * Refactor runner such that existing test and newly-added test are
>    subtests of 'ringbuf' top-level test (Yonghong)
> * Remove unused globals in test prog (Yonghong)
> 
>   tools/testing/selftests/bpf/Makefile          |  8 ++-
>   .../selftests/bpf/prog_tests/ringbuf.c        | 63 ++++++++++++++++-
>   .../bpf/progs/test_ringbuf_map_key.c          | 70 +++++++++++++++++++
>   3 files changed, 137 insertions(+), 4 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> 
[...]
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> new file mode 100644
> index 000000000000..495f85c6e120
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_map_key.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +struct sample {
> +	int pid;
> +	int seq;
> +	long value;
> +	char comm[16];
> +};
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_RINGBUF);
> +	__uint(max_entries, 4096);
> +} ringbuf SEC(".maps");
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__uint(max_entries, 1000);
> +	__type(key, struct sample);
> +	__type(value, int);
> +} hash_map SEC(".maps");
> +
> +/* inputs */
> +int pid = 0;
> +
> +/* inner state */
> +long seq = 0;
> +
> +SEC("fentry/" SYS_PREFIX "sys_getpgid")
> +int test_ringbuf_mem_map_key(void *ctx)
> +{
> +	int cur_pid = bpf_get_current_pid_tgid() >> 32;
> +	struct sample *sample, sample_copy;
> +	int *lookup_val;
> +
> +	if (cur_pid != pid)
> +		return 0;
> +
> +	sample = bpf_ringbuf_reserve(&ringbuf, sizeof(*sample), 0);
> +	if (!sample)
> +		return 0;
> +
> +	sample->pid = pid;
> +	bpf_get_current_comm(sample->comm, sizeof(sample->comm));
> +	sample->seq = ++seq;
> +	sample->value = 42;
> +
> +	/* test using 'sample' (PTR_TO_MEM | MEM_ALLOC) as map key arg
> +	 */
> +	lookup_val = (int *)bpf_map_lookup_elem(&hash_map, sample);
> +
> +	/* memcpy is necessary so that verifier doesn't complain with:
> +	 *   verifier internal error: more than one arg with ref_obj_id R3
> +	 * when trying to do bpf_map_update_elem(&hash_map, sample, &sample->seq, BPF_ANY);
> +	 *
> +	 * Since bpf_map_lookup_elem above uses 'sample' as key, test using
> +	 * sample field as value below
> +	 */

If I understand correctly, the above error is due to the following 
verifier code:

         if (reg->ref_obj_id) {
                 if (meta->ref_obj_id) {
                         verbose(env, "verifier internal error: more 
than one arg with ref_obj_id R%d %u %u\n",
                                 regno, reg->ref_obj_id,
                                 meta->ref_obj_id);
                         return -EFAULT;
                 }
                 meta->ref_obj_id = reg->ref_obj_id;
         }

So this is an internal error. So normally this should not happen.
Could you investigate and fix the issue?

> +	__builtin_memcpy(&sample_copy, sample, sizeof(struct sample));
> +	bpf_map_update_elem(&hash_map, &sample_copy, &sample->seq, BPF_ANY);
> +
> +	bpf_ringbuf_submit(sample, 0);
> +	return 0;
> +}