| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <8A43B87B-A478-4AA1-8154-D459D25B3320@linux.ibm.com>
Date: Tue, 18 Nov 2025 15:17:01 +0530
From: Venkat <venkat88@...ux.ibm.com>
To: Saket Kumar Bhaskar <skb99@...ux.ibm.com>
Cc: bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>,
Hari Bathini <hbathini@...ux.ibm.com>, sachinpb@...ux.ibm.com,
andrii@...nel.org, eddyz87@...il.com, ast@...nel.org,
daniel@...earbox.net, martin.lau@...ux.dev, song@...nel.org,
yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org,
sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org
Subject: Re: [PATCH bpf-next v3] selftests/bpf: Fix htab_update/reenter_update
selftest failure
> On 17 Nov 2025, at 11:37 AM, Saket Kumar Bhaskar <skb99@...ux.ibm.com> wrote:
>
> Since commit 31158ad02ddb ("rqspinlock: Add deadlock detection
> and recovery") the updated path on re-entrancy now reports deadlock
> via -EDEADLK instead of the previous -EBUSY.
>
> Also, the way reentrancy was exercised (via fentry/lookup_elem_raw)
> has been fragile because lookup_elem_raw may be inlined
> (find_kernel_btf_id() will return -ESRCH).
>
> To fix this fentry is attached to bpf_obj_free_fields() instead of
> lookup_elem_raw() and:
>
> - The htab map is made to use a BTF-described struct val with a
> struct bpf_timer so that check_and_free_fields() reliably calls
> bpf_obj_free_fields() on element replacement.
>
> - The selftest is updated to do two updates to the same key (insert +
> replace) in prog_test.
>
> - The selftest is updated to align with expected errno with the
> kernel’s current behavior.
>
> Signed-off-by: Saket Kumar Bhaskar <skb99@...ux.ibm.com>
Tested this patch by applying on top of bpd-next and it works as expected. Please add below tag.
Tested-by: Venkat Rao Bagalkote <venkat88@...ux.ibm.com>
With this change:
./test_progs -t htab_update
#144/1 htab_update/reenter_update:OK
#144/2 htab_update/concurrent_update:OK
#144 htab_update:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
Regards,
Venkat.
> ---
> Changes since v2:
> Addressed CI failures:
> * Initialize key to 0 before the first update.
> * Used pointer value to pass for update and memset rather than
> &value.
>
> v2: https://lore.kernel.org/all/20251114152653.356782-1-skb99@linux.ibm.com/
>
> Changes since v1:
> Addressed comments from Alexei:
> * Fixed the scenario where test may fail when lookup_elem_raw()
> is inlined.
>
> v1: https://lore.kernel.org/all/20251106052628.349117-1-skb99@linux.ibm.com/
>
> .../selftests/bpf/prog_tests/htab_update.c | 37 ++++++++++++++-----
> .../testing/selftests/bpf/progs/htab_update.c | 19 +++++++---
> 2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/htab_update.c b/tools/testing/selftests/bpf/prog_tests/htab_update.c
> index 2bc85f4814f4..d0b405eb2966 100644
> --- a/tools/testing/selftests/bpf/prog_tests/htab_update.c
> +++ b/tools/testing/selftests/bpf/prog_tests/htab_update.c
> @@ -15,17 +15,17 @@ struct htab_update_ctx {
> static void test_reenter_update(void)
> {
> struct htab_update *skel;
> - unsigned int key, value;
> + void *value = NULL;
> + unsigned int key, value_size;
> int err;
>
> skel = htab_update__open();
> if (!ASSERT_OK_PTR(skel, "htab_update__open"))
> return;
>
> - /* lookup_elem_raw() may be inlined and find_kernel_btf_id() will return -ESRCH */
> - bpf_program__set_autoload(skel->progs.lookup_elem_raw, true);
> + bpf_program__set_autoload(skel->progs.bpf_obj_free_fields, true);
> err = htab_update__load(skel);
> - if (!ASSERT_TRUE(!err || err == -ESRCH, "htab_update__load") || err)
> + if (!ASSERT_TRUE(!err, "htab_update__load") || err)
> goto out;
>
> skel->bss->pid = getpid();
> @@ -33,14 +33,33 @@ static void test_reenter_update(void)
> if (!ASSERT_OK(err, "htab_update__attach"))
> goto out;
>
> - /* Will trigger the reentrancy of bpf_map_update_elem() */
> + value_size = bpf_map__value_size(skel->maps.htab);
> +
> + value = calloc(1, value_size);
> + if (!ASSERT_OK_PTR(value, "calloc value"))
> + goto out;
> + /*
> + * First update: plain insert. This should NOT trigger the re-entrancy
> + * path, because there is no old element to free yet.
> + */
> key = 0;
> - value = 0;
> - err = bpf_map_update_elem(bpf_map__fd(skel->maps.htab), &key, &value, 0);
> - if (!ASSERT_OK(err, "add element"))
> + err = bpf_map_update_elem(bpf_map__fd(skel->maps.htab), &key, value, BPF_ANY);
> + if (!ASSERT_OK(err, "first update (insert)"))
> + goto out;
> +
> + /*
> + * Second update: replace existing element with same key and trigger
> + * the reentrancy of bpf_map_update_elem().
> + * check_and_free_fields() calls bpf_obj_free_fields() on the old
> + * value, which is where fentry program runs and performs a nested
> + * bpf_map_update_elem(), triggering -EDEADLK.
> + */
> + memset(value, 0, value_size);
> + err = bpf_map_update_elem(bpf_map__fd(skel->maps.htab), &key, value, BPF_ANY);
> + if (!ASSERT_OK(err, "second update (replace)"))
> goto out;
>
> - ASSERT_EQ(skel->bss->update_err, -EBUSY, "no reentrancy");
> + ASSERT_EQ(skel->bss->update_err, -EDEADLK, "no reentrancy");
> out:
> htab_update__destroy(skel);
> }
> diff --git a/tools/testing/selftests/bpf/progs/htab_update.c b/tools/testing/selftests/bpf/progs/htab_update.c
> index 7481bb30b29b..195d3b2fba00 100644
> --- a/tools/testing/selftests/bpf/progs/htab_update.c
> +++ b/tools/testing/selftests/bpf/progs/htab_update.c
> @@ -6,24 +6,31 @@
>
> char _license[] SEC("license") = "GPL";
>
> +/* Map value type: has BTF-managed field (bpf_timer) */
> +struct val {
> + struct bpf_timer t;
> + __u64 payload;
> +};
> +
> struct {
> __uint(type, BPF_MAP_TYPE_HASH);
> __uint(max_entries, 1);
> - __uint(key_size, sizeof(__u32));
> - __uint(value_size, sizeof(__u32));
> + __type(key, __u32);
> + __type(value, struct val);
> } htab SEC(".maps");
>
> int pid = 0;
> int update_err = 0;
>
> -SEC("?fentry/lookup_elem_raw")
> -int lookup_elem_raw(void *ctx)
> +SEC("?fentry/bpf_obj_free_fields")
> +int bpf_obj_free_fields(void *ctx)
> {
> - __u32 key = 0, value = 1;
> + __u32 key = 0;
> + struct val value = { .payload = 1 };
>
> if ((bpf_get_current_pid_tgid() >> 32) != pid)
> return 0;
>
> - update_err = bpf_map_update_elem(&htab, &key, &value, 0);
> + update_err = bpf_map_update_elem(&htab, &key, &value, BPF_ANY);
> return 0;
> }
> --
> 2.51.0
Powered by blists - more mailing lists