> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> new file mode 100644
> index 000000000000..29de05bcbd4b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
[ ... ]
> +static void subtest_timer(struct rhtab_fields *skel)
> +{
> + u64 zero = 0;
> + u32 key = 0;
> + int fired, map_fd;
> +
> + map_fd = bpf_map__fd(skel->maps.tmap);
> + if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
> + "create_elem"))
> + return;
Does this read past the end of the zero variable? The tmap value type
is struct timer_val which is 24 bytes (16-byte bpf_timer plus 8-byte
data field), but zero is only 8 bytes. The syscall copies value_size
bytes from the user pointer in kernel/bpf/syscall.c:
value_size = bpf_map_value_size(map);
value = kvmemdup_bpfptr(uvalue, value_size);
so it appears 16 bytes above zero on the stack would be read. The same
pattern is also used later when recreating the element.
> +
> + if (!ASSERT_OK(run_prog(skel, "arm_timer"), "arm_timer_first"))
> + return;
> + usleep(300000);
> + if (!ASSERT_GT(skel->bss->timer_fired, 0, "timer_fired_first"))
> + return;
> +
> + /* Deleting the element must cancel the timer. */
> + fired = skel->bss->timer_fired;
> + if (!ASSERT_OK(bpf_map_delete_elem(map_fd, &key), "delete_elem"))
> + return;
> + usleep(300000);
> + ASSERT_EQ(skel->bss->timer_fired, fired,
> "timer_cancelled_after_delete");
Can this assertion actually verify timer cancellation? Looking at
arm_timer() in progs/rhtab_fields.c, it arms the timer with a 50us
expiry (50000ns), and the callback doesn't re-arm. The test waits
300ms before the delete, and the preceding ASSERT_GT confirms the timer
has already fired. So by the time bpf_map_delete_elem() runs, the
timer has been inactive for ~300ms. Whether or not the delete path
cancels the timer field, timer_fired cannot advance during the second
sleep, so the assertion would pass even on a kernel that doesn't cancel
the timer at all.
The commit message says "delete the element and verify the timer is
cancelled" - to actually test cancellation, wouldn't the timer need to
still be pending at delete time? For example, arming it with a delay
longer than the deletion (say 200ms), deleting immediately, then
sleeping past the original expiry and checking that timer_fired did not
change.
> diff --git a/tools/testing/selftests/bpf/progs/rhtab_fields.c
> b/tools/testing/selftests/bpf/progs/rhtab_fields.c
> new file mode 100644
> index 000000000000..85335f19f172
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c
[ ... ]
> +struct timer_val {
> + struct bpf_timer timer;
> + __u64 data;
> +};
Is the data field used anywhere? I don't see it read or written in
any of the programs or in the test harness. Compare struct
lock_kptr_val::magic which exists for a similar purpose (plain bytes
alongside a special field) and is actually exercised by lk_upd/lk_probe
to verify plain bytes round-trip through recycled elements.
The field does affect the value_size (making it 24 bytes instead of
16), which creates a mismatch with prog_tests/rhtab_fields.c where
subtest_timer() seeds the map from an 8-byte local (u64 zero).
> +} counters SEC(".maps");
> +
> +/* 0: lk init ok, 1: lk probe xchg non-NULL, 2: lk probe xchg NULL,
> + * 3: lk probe magic ok, 4: u init ok, 5: u probe ptr non-NULL,
> + * 6: pc init ok, 7: pc probe xchg non-NULL, 8: pc probe xchg NULL
> + */
This isn't a bug, but the comment style here differs from the file
header above. The BPF subsystem prefers the opening /* on its own
line:
/*
* 0: lk init ok, 1: lk probe xchg non-NULL, ...
*/
> +static __always_inline void bump(u32 idx)
> +{
> + u64 *v = bpf_map_lookup_elem(&counters, &idx);
> +
> + if (v)
> + (*v)++;
> +}
> +
> +extern struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym;
> +extern void bpf_task_release(struct task_struct *p) __ksym;
> +
> +int timer_fired;
> +
> +/* Map 1: spin lock + referenced kptr + plain data. */
> +
> +SEC("syscall")
> +int lk_init(void *ctx)
> +{
> + struct lock_kptr_val *val;
> + struct task_struct *task, *old;
> + u32 key = 0;
> +
> + val = bpf_map_lookup_elem(&lkmap, &key);
> + if (!val)
> + return 1;
> + task = bpf_task_acquire(bpf_get_current_task_btf());
> + if (!task)
> + return 2;
> + old = bpf_kptr_xchg(&val->tsk, task);
> + if (old)
> + bpf_task_release(old);
> + bump(0);
> + return 0;
> +}
> +
> +SEC("syscall")
> +int lk_del(void *ctx)
> +{
> + u64 key = 0;
> +
> + bpf_map_delete_elem(&lkmap, &key);
> + return 0;
> +}
> +
> +SEC("syscall")
> +int lk_upd(void *ctx)
> +{
> + struct lock_kptr_val val = { .magic = 0x52484142 };
> + u32 key = 0;
> +
> + bpf_map_update_elem(&lkmap, &key, &val, BPF_ANY);
> + return 0;
> +}
> +
> +SEC("syscall")
> +int lk_probe(void *ctx)
> +{
> + struct lock_kptr_val *val;
> + struct task_struct *old;
> + u32 key = 0;
> +
> + val = bpf_map_lookup_elem(&lkmap, &key);
> + if (!val)
> + return 1;
> + old = bpf_kptr_xchg(&val->tsk, NULL);
> + if (old) {
> + bpf_task_release(old);
> + bump(1);
> + } else {
> + bump(2);
> + }
> + if (val->magic == 0x52484142)
> + bump(3);
> + return 0;
> +}
A subsystem pattern flags this as potentially concerning: the
lk_init/lk_del/lk_upd/lk_probe quartet appears to re-implement a test
that the immediately preceding commit in this same series (81f409ba449d)
just added to the same directory. progs/rhtab_kptr.c already covers
the rhtab referenced-kptr recycle path with an identical four-program
template over the same map type.
The shared scaffolding is duplicated as well: bump() and the extern
bpf_task_acquire/bpf_task_release declarations are byte-identical
between the two files.
Given that rhtab_fields.c adds the spin-lock, timer, untrusted-kptr,
and per-cpu-kptr combinations that rhtab_kptr.c doesn't cover, while
rhtab_kptr.c covers the NMI (perf_event) update path that
rhtab_fields.c doesn't, neither file strictly subsumes the other.
Should the two files share a header for the common helpers, or would it
make sense to merge them into one test that covers all field
combinations including the NMI case?
> +
> +/* Map 2: bpf_timer. */
> +
> +static int timer_cb(void *map, void *key, struct timer_val *value)
> +{
> + timer_fired++;
> + return 0;
> +}
> +
> +SEC("syscall")
> +int arm_timer(void *ctx)
> +{
> + struct timer_val *val;
> + u32 key = 0;
> +
> + val = bpf_map_lookup_elem(&tmap, &key);
> + if (!val)
> + return 1;
> + /* 1 == CLOCK_MONOTONIC */
> + if (bpf_timer_init(&val->timer, &tmap, 1))
> + return 2;
> + bpf_timer_set_callback(&val->timer, timer_cb);
> + if (bpf_timer_start(&val->timer, 50000, 0))
> + return 3;
> + return 0;
> +}
Does arming a one-shot timer with a 50us expiry make the timer subtest's
delete-cancellation assertion unfalsifiable? The callback doesn't
re-arm, so the timer fires exactly once. The consumer in
prog_tests/rhtab_fields.c waits 300ms (6000x the expiry) and confirms
the timer has already fired before calling bpf_map_delete_elem(). The
hrtimer is therefore inactive at delete time, so timer_fired cannot
change during the second sleep regardless of whether the delete path
actually cancels anything.
The existing convention in this directory is to arm a long timer so
it is still pending at delete time - progs/timer_start_delete_race.c
uses bpf_timer_start(&value->timer, 100000000, 0) (100ms) for exactly
this delete-vs-pending-timer scenario, and progs/timer.c uses
1ull << 35 (~34s) as its 'must not fire' expiry. Would arming with an
expiry longer than the arm-to-delete window make the assertion able to
fail?
[ ... ]
---
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/32741782570
