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/22/22 7:27 AM, Dave Marchevsky wrote:
> On 9/20/22 1:50 AM, Yonghong Song wrote:
>>
>>
>> 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.
>>
> 
> The bpf_map_lookup_elem() usecase is the only one critical for me, so I've
> submitted v3 without ref_obj_id fix. I agree that it should be fixed, but feels
> orthogonal to this change, and is probably best addressed as a verifier-wide
> fix affecting all functions as per Kumar's suggestion.

Okay. This works for me. The ref_obj_id fix can be a followup.