Re: [PATCH bpf 2/2] selftests/bpf: Add test for key-less BTF hash map

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_map_keyless.c b/tools/testing/selftests/bpf/prog_tests/btf_map_keyless.c
> new file mode 100644
> index 000000000000..6e1496cbd8bf
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_map_keyless.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <bpf/btf.h>

This isn't a bug, but tools/testing/selftests/bpf/prog_tests/btf.c already
has a table-driven harness for map-creation-rejection cases. do_test_raw()
(around btf.c:4519-4530) does:

    opts.btf_key_type_id = test->key_type_id;
    opts.btf_value_type_id = test->value_type_id;
    map_fd = bpf_map_create(test->map_type, test->map_name, ...);
    err = ((map_fd < 0) != test->map_create_err);

and struct btf_raw_test already carries .map_type, .key_size, .value_size,
.max_entries, .key_type_id, .value_type_id and .map_create_err, with six
existing .map_create_err = true entries already in that table. A key-less
hash/rhash case could be two more rows in that table rather than a new file.
Is there a reason to create a new file instead of adding cases to the
existing test?

> +
> +/*
> + * A key-less BTF (btf_key_type_id == 0) is only meant for array maps. A hash
> + * map with one used to NULL-deref in btf_type_show() when dumped via bpffs.
> + * A fixed kernel rejects such a map at creation; on an unfixed kernel the
> + * pin-and-read below deliberately walks that bpffs dump path, so running this
> + * test on an unfixed kernel panics it.
> + */
> +static void check_keyless(int map_type, __u32 map_flags, int btf_fd, int val_id)
> +{
> +	const char *path = "/sys/fs/bpf/keyless_map";
> +	LIBBPF_OPTS(bpf_map_create_opts, opts);
> +	__u32 key = 1, val = 0x41424344;
> +	char buf[256];
> +	int map_fd;
> +	FILE *f;
> +
> +	opts.map_flags = map_flags;
> +	opts.btf_fd = btf_fd;
> +	opts.btf_key_type_id = 0;
> +	opts.btf_value_type_id = val_id;
> +	map_fd = bpf_map_create(map_type, "keyless_map", 4, 4, 8, &opts);
> +
> +	if (map_fd >= 0) {
> +		/* Unfixed kernel: reproduce the oops via the bpffs dump path. */
> +		(void)bpf_map_update_elem(map_fd, &key, &val, 0);
> +		if (bpf_obj_pin(map_fd, path) == 0) {
> +			f = fopen(path, "r");
> +			if (f) {
> +				while (fgets(buf, sizeof(buf), f))
> +					;
> +				fclose(f);
> +			}
> +			unlink(path);
> +		}
> +		close(map_fd);
> +	}

Does this block execute the kernel NULL-deref before the assertion can fail?

On any kernel lacking the companion fix f763f12b26fc ("bpf: Fix
NULL-ptr-deref when dumping a key-less BTF hash map") -- every kernel before
this series, any tree bisected between patch 1 and patch 2, and any backport
that takes the selftest without the kernel hunk -- bpf_map_create() succeeds,
so map_fd >= 0 and the block runs. It inserts an element, pins the map into
bpffs, and then reads the pin, which walks exactly the path quoted in the
fix's own splat:

  seq_read -> map_seq_show (kernel/bpf/inode.c)
           -> htab_map_seq_show_elem (kernel/bpf/hashtab.c)
           -> btf_type_seq_show
           -> btf_type_show (kernel/bpf/btf.c:8229)

with map->btf_key_type_id == 0, where btf_type_by_id() yields &btf_void and
kind_ops[BTF_KIND_UNKN] is NULL, so btf_type_ops(t)->show is a
NULL-pointer read.

That is an in-kernel oops in process context: the reading task (the
test_progs process, or one of its -j workers) is killed, so ASSERT_EQ()
below never executes and the harness reports a dead process rather than a
clean subtest failure. On any host booted with panic_on_oops (the norm for
BPF CI / syzkaller configs; tools/testing/selftests/bpf/config.ppc64el
already sets panic_on_warn=1 in CONFIG_CMDLINE) the machine panics and the
remainder of the test_progs run is lost.

ASSERT_EQ(map_fd, -EINVAL, ...) below already fails on an unfixed kernel,
because an unfixed kernel returns a valid fd instead of -EINVAL. On a fixed
kernel this block is unreachable. A selftest should detect the regression,
not trigger it; should the pin-and-read block be dropped (or at most gated
behind an explicit opt-in)?

> +
> +	ASSERT_EQ(map_fd, -EINVAL, "key-less create is rejected");

Does this assertion pass for the right reason?

-EINVAL is the generic BPF_MAP_CREATE rejection code, so this passes for
many reasons that have nothing to do with the key-less BTF check being
present.

Concretely for the "rhash" subtest: on any kernel that does not know
BPF_MAP_TYPE_RHASH (the map type is new in this tree; the selftest is built
from tools/include/uapi/linux/bpf.h, not from the running kernel's),
find_and_alloc_map() returns ERR_PTR(-EINVAL) because bpf_map_types[type] is
NULL or type is out of range. The subtest then passes without ever reaching
rhtab_map_check_btf().

Likewise, rhtab_map_alloc_check() returns -EINVAL for a missing
BPF_F_NO_PREALLOC / bad map_extra, and map_check_btf() returns -EINVAL if
the value type size ever stops matching value_size (4). Any of these makes
the subtest a green tick that pins nothing.

Is there a positive control? The test never asserts that the same
bpf_map_create() call succeeds when btf_key_type_id is set to a real type
id. Adding that (or checking for the create-time BTF log / distinguishing
the failure source) is what separates "the kernel rejected a key-less hash
map" from "the kernel rejected this map for some unrelated reason."

[ ... ]

The commit message states:

> Create a hash and an rhash map with btf_key_type_id == 0 and expect
> bpf_map_create() to fail with -EINVAL. On an unfixed kernel the map is
> created; the test then pins and reads it back to walk the bpffs dump path,
> which reproduces the btf_type_show() NULL-deref - so running this test on
> an unfixed kernel panics it.

Is "panics" accurate? What the traced path actually produces is an oops (a
NULL function-pointer read in btf_type_show() reached from seq_read() in
process context), which makes the reading task dead; the box only panics if
it was booted with panic_on_oops. More importantly, the message presents the
deliberate crash as the intended behaviour of the test rather than
justifying why the reproducer is kept in-tree when ASSERT_EQ(map_fd,
-EINVAL) alone already detects the regression. The commit also carries no
Fixes: tag, which is consistent with it being the selftest half of the
series, but a reviewer reading it in isolation gets no signal that the test
will take down a kernel that lacks the companion patch.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33161022932