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@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.
Okay. This works for me. The ref_obj_id fix can be a followup.
