Misc bug fixes - part 4

tools/testing/selftests/bpf/prog_tests/rhash_timer.c line 71

[Severity: Low]
According to the BPF subsystem guidelines for the skeleton API, after a successful rhash_timer__open_and_load() call, the skeleton pointer is valid and all maps are guaranteed to be created with valid FDs. Is it necessary to manually extract and validate map_fd using ASSERT_GE() here?

tools/testing/selftests/bpf/prog_tests/rhash_timer.c line 77

[Severity: Low]
Similarly, is this program FD validation needed? The skeleton guarantees that all programs are loaded with valid FDs after open_and_load succeeds, so bpf_program__fd() cannot return a negative value here.

[ ... ]

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

tools/testing/selftests/bpf/prog_tests/rhash_timer.c line 7

A subsystem pattern flags this as potentially concerning: two brand-new standalone selftest files are added for one narrow internal rhtab code path - preservation of the BPF_TIMER special field when a deleted rhtab element's bpf_mem_alloc unit is recycled by an insert.

tools/testing/selftests/bpf/prog_tests/rhash.c (183 lines, subtests rhash_run, rhash_map_extra_presize, rhash_map_extra_too_big, rhash_iter_test) and tools/testing/selftests/bpf/progs/rhash.c (248 lines, 7 SEC("syscall") programs) already exist in the same two directories and cover BPF_MAP_TYPE_RHASH behaviour generally, including map creation, update/lookup/delete and iteration.

Same-series commit e5013fee53d73 adds rhtab special-field (kptr) coverage as new cases inside tools/testing/selftests/bpf/prog_tests/ rhash.c and tools/testing/selftests/bpf/progs/rhash.c rather than as a new file, which is direct evidence of the convention this change departs from.

Would adding a fifth subtest to test_rhash() plus one more SEC("syscall") pair in progs/rhash.c give the same coverage without a new test binary entry, a second skeleton and a duplicated map definition?

[ ... ]

tools/testing/selftests/bpf/prog_tests/rhash_timer.c line 105

This isn't a bug, but can the three diagnostic checks for timer_init_err, timer_set_callback_err and timer_start_err ever fire?

The armed counter check precedes them and catches every failure that sets those variables. In progs/rhash_timer.c:arm_timer_cb() each of the three error paths records the errno and returns 1 without running __sync_fetch_and_add(&armed, 1), so any of those failures leaves armed behind attempt + 1, the ASSERT_EQ(skel->bss->armed, attempt + 1, "armed") fails, and the function goes to out before reaching the three errno checks.

Armed can never catch up in a later iteration either, since it increments at most once per arm run. The net effect is that the three per-helper errno variables - the whole point of which is a precise failure message - are unreachable, and a bpf_timer_init() failure reports the uninformative "armed 3 != 4".

The analogous timer_cancel_err check later in the loop is reachable, because cancel_timer_cb() records the errno without incrementing cancelled and the test checks timer_cancel_err before it checks cancelled.

Would checking the three errno variables before the armed count (as is already done for timer_cancel_err vs cancelled) make the diagnostic messages reachable?

[ ... ]

This isn't a bug, but inside the loop the same three-line shape is repeated nine times:

	if (cond) {
		ASSERT_x(...);
		goto out;
	}

instead of the idiomatic one-liner:

	if (!ASSERT_x(...))
		goto out;

The occurrences are in the loop body where the test checks err, skel->bss->armed, timer_init_err, timer_set_callback_err, timer_start_err, kern_sync_rcu, update_timer_map, run_prog for cancel, and timer_cancel_err.

The same function already uses the idiomatic form six times immediately above (after pin_to_first_cpu, after open_and_load, for the three fd assignments, and after seed_timer_map), so the file is internally inconsistent and the loop body is roughly twice as long as it needs to be.

Effectively all of tools/testing/selftests/bpf/prog_tests/ uses if (!ASSERT_x(...)) goto/return, including prog_tests/rhash.c which this test sits beside. Could the nine blocks be collapsed to
if (!ASSERT_x(...)) goto out?

[ ... ]

---
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/33865340224

kernel/bpf/hashtab.c static int rhtab_delete_elem(struct bpf_rhtab *rhtab, struct rhtab_elem *elem, v · line 2962

[Severity: High]
Does changing this to bpf_obj_cancel_fields() leave stale special fields in the memory cache?

Since bpf_obj_cancel_fields() does not drop kptrs or list heads, these references appear to remain in the element when it is freed to the cache. When a new element is later allocated from the cache in rhtab_map_update_elem():

    elem = bpf_mem_cache_alloc(&rhtab->ma);
    if (!elem)
        return -ENOMEM;
    memcpy(elem->data, key, map->key_size);
    copy_map_value(map, rhtab_elem_value(elem, map->key_size), value);

Because bpf_mem_cache_alloc() does not zero memory and copy_map_value() skips special fields, will this cause the new key to inherit the old key's populated list head or kptr?

kernel/bpf/hashtab.c static long rhtab_map_update_existing(struct bpf_map *map, struct rhtab_elem *el · line 3021

[Severity: High]
Does replacing the free operation with bpf_obj_cancel_fields() here violate the requirement to clean up special BTF fields during map updates?

When overwriting an existing map value in-place, failing to free existing kptrs or list heads seems like it causes them to outlive the update. Will this leave stale resources bound to the key indefinitely?

kernel/bpf/hashtab.c static long rhtab_map_update_existing(struct bpf_map *map, struct rhtab_elem *el · line 3023

A subsystem pattern flags this as potentially concerning: the BPF field semantics guide states that map operations on field-capable map types that copy values with copy_map_value() should use the corresponding bpf_obj_free_fields() on updates. Should this update path still call bpf_obj_free_fields() to handle kptr references?

rhtab_map_update_existing() overwrites a live map value in place with copy_map_value_locked() at kernel/bpf/hashtab.c:3008-3010 and then calls bpf_obj_cancel_fields(). Looking at kernel/bpf/helpers.c:5033, bpf_obj_cancel_fields() resolves to bpf_map_free_internal_structs() which only handles BPF_TIMER, BPF_WORKQUEUE and BPF_TASK_WORK:

void bpf_map_free_internal_structs(struct bpf_map *map, void *val)

{
        if (btf_record_has_field(map->record, BPF_TIMER))
                bpf_obj_free_timer(map->record, val);
        if (btf_record_has_field(map->record, BPF_WORKQUEUE))
                bpf_obj_free_workqueue(map->record, val);
        if (btf_record_has_field(map->record, BPF_TASK_WORK))
                bpf_obj_free_task_work(map->record, val);
}

BPF_MAP_TYPE_RHASH appears in the map_check_btf() allowlists for BPF_KPTR_UNREF, BPF_KPTR_REF, BPF_KPTR_PERCPU and BPF_REFCOUNT in kernel/bpf/syscall.c:1322-1338. After this change a BPF_EXIST or BPF_ANY update of an existing rhtab element no longer immediately drops the referenced kptr held by the value being replaced, and no longer zeroes an unref kptr - the previous value's kptr stays visible through the new value until the allocation is reclaimed by rhtab_mem_dtor().

Is this semantic change intentional? Looking at the current tree, array_map_update_elem() also calls bpf_obj_cancel_fields() in kernel/bpf/arraymap.c:419 and :427, and the non-prealloc htab free path uses check_and_cancel_fields() via kernel/bpf/hashtab.c:843, so the bpf_obj_free_fields()-on-update pattern in the guide may be stale relative to these implementations. The allocator itself appears safe: bpf_ma_set_dtor() installs rhtab_mem_dtor() as the bpf_mem_alloc destructor, and every allocator exit path funnels through free_all() at kernel/bpf/memalloc.c:265-277 which invokes the destructor, so kptr references should be released when the allocation is finally destroyed. Is the intent that rhtab now matches arraymap's deferred-release behaviour, and if so, should the subsystem documentation be updated to reflect this?

Does the comment at kernel/bpf/hashtab.c:3017-3019 still accurately describe what the code does? It states:

/*
* copy_map_value() skips special-field offsets, so old timers/
* kptrs/etc. still sit in the slot. Cancel them after the copy
* to match arraymap's update semantics.
*/

The comment says old "timers/kptrs/etc." are cancelled after the copy, but bpf_obj_cancel_fields() only cancels BPF_TIMER, BPF_WORKQUEUE and BPF_TASK_WORK. Referenced kptrs are deliberately left in the slot - that is the whole point of this commit ("allowing rhtab_mem_dtor() to release referenced kptrs when the allocation is eventually destroyed") and is asserted by the follow-up selftest e5013fee53d7. A reader of this function is told the opposite of what the code does. Should the comment be updated to say that only the asynchronous fields are cancelled here and that referenced kptrs stay attached to the allocation until rhtab_mem_dtor() runs?

---
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/33865340224

kernel/bpf/verifier.c static int check_map_kptr_access(struct bpf_verifier_env *env, · line 4704

[Severity: High]
This is a pre-existing issue, and was not introduced by the patch under review, but while this commit successfully fixes the precision-tracking bug in check_map_kptr_access(), does the exact same vulnerability still exist in several other locations where bpf_register_is_null() is used without calling mark_chain_precision()?

For example, in check_func_arg():

	if (bpf_register_is_null(reg) && type_may_be_null(arg_type))
		/* A NULL register has a SCALAR_VALUE type, so skip
		 * type checking.
		 */
		goto skip_type_check;

And also later in check_func_arg() for map values:

	case ARG_PTR_TO_MAP_VALUE:
		if (type_may_be_null(arg_type) && bpf_register_is_null(reg))
			return 0;

Similarly, in btf_check_func_arg_match():

			if (bpf_register_is_null(reg) && type_may_be_null(arg->arg_type))
				continue;

And in release_reg():

	if (bpf_register_is_null(reg))
		return 0;

And in check_kfunc_args() for nullable args:

		if (bpf_register_is_null(reg) && type_may_be_null(kf_arg_type))
			continue;

And in check_kfunc_args() for memory buffer lengths:

			if (bpf_register_is_null(buff_reg))
				break;

And in check_mem_reg():

	if (bpf_register_is_null(reg))
		return 0;

And in check_helper_call() for get_local_storage():

		if (!bpf_register_is_null(&regs[BPF_REG_2])) {
			verbose(env, "get_local_storage() doesn't support non-zero flags\n");
			return -EINVAL;
		}
		break;

Since mark_chain_precision() is omitted in these bypass paths, the scalar 0 remains imprecise. Could an attacker pass an attacker-controlled arbitrary scalar on a second pruned path, which would then be incorrectly accepted by the verifier when substituted at runtime?

tools/testing/selftests/bpf/progs/map_kptr_fail.c int reject_scalar_store_to_kptr(struct __sk_buff *ctx) · line 431

This isn't a bug, but would a one-line comment above the padding help - something noting that the ten filler insns are there to get a state checkpoint recorded at l1_%= (the >=8 insns / >=2 jumps heuristic), so a future reader knows the test silently loses coverage if that block shrinks?

tools/testing/selftests/bpf/progs/map_kptr_fail.c int reject_scalar_store_to_kptr(struct __sk_buff *ctx) · line 449

---
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/33865340224

tools/testing/selftests/bpf/prog_tests/timer_mim.c void serial_test_timer_mim(void) · line 84

Does this handle CONFIG_PREEMPT_RT correctly?

The callback_map_uid_match program calls bpf_timer_init(), which the verifier rejects unconditionally on PREEMPT_RT kernels:

kernel/bpf/verifier.c:process_timer_func() {
    if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
        verbose(env, "bpf_timer cannot be used for PREEMPT_RT.\n");
        return -EOPNOTSUPP;
    }

On a PREEMPT_RT build timer_mim_reject__load() returns -EOPNOTSUPP and ASSERT_OK(err, "callback_accept_skel_load") reports a test failure.

The existing skip hatch is a few lines later:

    timer_skel = timer_mim__open_and_load();
    if (!timer_skel && errno == EOPNOTSUPP) {
            test__skip();
            return;
    }

Before this patch all timer_mim_reject loads were expected to fail, so -EOPNOTSUPP satisfied the rejection assertion and control reached the skip. After this patch the must-succeed load fires first, converting timer_mim from SKIP to FAIL on PREEMPT_RT.

Should the capability check move ahead of the new positive control, or should callback_accept_skel_load check for err == -EOPNOTSUPP and call test__skip()?

Two related issues also follow from this ordering:

- libbpf_set_print(NULL) is still active when the positive control fails, so the verifier log is suppressed and only "unexpected error:
-95" is shown.

- The two ASSERT_ERR() arms for test1 and callback_map_uid_mismatch only check err < 0, so on PREEMPT_RT they also pass for the wrong reason (-EOPNOTSUPP instead of the intended map_uid mismatch), making those negative cases silent tautologies.

tools/testing/selftests/bpf/progs/timer_mim_reject.c int BPF_PROG(test1, int a) · line 156

---
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/33865340224