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

[Dave Marchevsky <davemarchevsky@meta.com>]

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.