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.
But it can probably be a separate change from this.
