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/19/22 4:22 PM, Kumar Kartikeya Dwivedi wrote:
> On Tue, 20 Sept 2022 at 00:53, Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> 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?
>>
> 
> Technically it's not an "internal" error, it's totally possible to
> pass two referenced registers from a program (which the verifier
> rejects). So a bad log message I guess.
> 
> We probably need to update the verifier to properly recognize the
> ref_obj_id for certain functions. For release arguments we already
> have meta.release_regno/OBJ_RELEASE for. It can already find the
> ref_obj_id from release_regno instead of meta.ref_obj_id.
> 
> For dynptr_ref or ptr_cast, simply store meta.ref_obj_id by capturing
> the regno and then setting it before r1-r5 is cleared.
> Since that is passed to r0 it will be done later after clearing of
> caller saved regs.
> ptr_cast and dynptr_ref functions are already exclusive (due to
> helper_multiple_ref_obj_use) so they can share the same regno field in
> meta.
> 
> Then remove this check on seeing more than one reg->ref_obj_id, so it
> isn't a problem to allow more than one refcounted registers for all
> other arguments, as long as we correctly remember the ones for the
> cases we care about.

Thanks for the explanation!

> 
> But it can probably be a separate change from this.

if the use case this patch set tried to address is using
bpf_map_update_elem(), we should fix the double
ref_obj_id in the current patch set. If only
bpf_map_lookup_elem() is needed. Then we can delay
the verifier change for the followup patch.