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@xxxxxx> 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@xxxxxx>
>>>> ---
>>>> v1->v2: lore.kernel.org/bpf/20220912101106.2765921-1-davemarchevsky@xxxxxx
>>>>
>>>> * 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.
