[PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Puranjay Mohan]

The __list_del fuction doesn't set the previous node's next pointer to
the next node of the node to be deleted. It just updates the local variable
and not the actual pointer in the previous node.

The test was passing up till now because the bpf code is doing bpf_free()
after list_del and therfore reading head->first from the userspace will
read all zeroes. But after arena_list_del() is finished, head->first should
point to NULL;

If you remove the bpf_free() call in arena_list_del(), the test will start
crashing because now the userpsace will read 0x100 (LIST_POISON1) in
head->first and segfault.

Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 tools/testing/selftests/bpf/bpf_arena_list.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
index 85dbc3ea4da5..e16fa7d95fcf 100644
--- a/tools/testing/selftests/bpf/bpf_arena_list.h
+++ b/tools/testing/selftests/bpf/bpf_arena_list.h
@@ -64,14 +64,12 @@ static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
 
 static inline void __list_del(arena_list_node_t *n)
 {
-	arena_list_node_t *next = n->next, *tmp;
+	arena_list_node_t *next = n->next;
 	arena_list_node_t * __arena *pprev = n->pprev;
 
 	cast_user(next);
 	cast_kern(pprev);
-	tmp = *pprev;
-	cast_kern(tmp);
-	WRITE_ONCE(tmp, next);
+	WRITE_ONCE(*pprev, next);
 	if (next) {
 		cast_user(pprev);
 		cast_kern(next);
-- 
2.47.3

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Yonghong Song]

On 10/17/25 7:17 AM, Puranjay Mohan wrote:
> The __list_del fuction doesn't set the previous node's next pointer to
> the next node of the node to be deleted. It just updates the local variable
> and not the actual pointer in the previous node.
>
> The test was passing up till now because the bpf code is doing bpf_free()
> after list_del and therfore reading head->first from the userspace will
> read all zeroes. But after arena_list_del() is finished, head->first should
> point to NULL;
>
> If you remove the bpf_free() call in arena_list_del(), the test will start
> crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> head->first and segfault.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Eduard Zingerman]

On Fri, 2025-10-17 at 14:17 +0000, Puranjay Mohan wrote:
> The __list_del fuction doesn't set the previous node's next pointer to
> the next node of the node to be deleted. It just updates the local variable
> and not the actual pointer in the previous node.
> 
> The test was passing up till now because the bpf code is doing bpf_free()
> after list_del and therfore reading head->first from the userspace will
> read all zeroes. But after arena_list_del() is finished, head->first should
> point to NULL;
> 
> If you remove the bpf_free() call in arena_list_del(), the test will start
> crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> head->first and segfault.

I tried commenting out bpf_free() in arena_list_del() but the test
passes for me even w/o this patch.  Is there a way to modify the test
case, so that logic of the list_del() is checked more thoroughly?

> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>

[...]

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Alexei Starovoitov]

On Fri, Oct 17, 2025 at 7:17 AM Puranjay Mohan <puranjay@kernel.org> wrote:
>
> The __list_del fuction doesn't set the previous node's next pointer to
> the next node of the node to be deleted. It just updates the local variable
> and not the actual pointer in the previous node.
>
> The test was passing up till now because the bpf code is doing bpf_free()
> after list_del and therfore reading head->first from the userspace will
> read all zeroes. But after arena_list_del() is finished, head->first should
> point to NULL;
>
> If you remove the bpf_free() call in arena_list_del(), the test will start
> crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> head->first and segfault.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>  tools/testing/selftests/bpf/bpf_arena_list.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
> index 85dbc3ea4da5..e16fa7d95fcf 100644
> --- a/tools/testing/selftests/bpf/bpf_arena_list.h
> +++ b/tools/testing/selftests/bpf/bpf_arena_list.h
> @@ -64,14 +64,12 @@ static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
>
>  static inline void __list_del(arena_list_node_t *n)
>  {
> -       arena_list_node_t *next = n->next, *tmp;
> +       arena_list_node_t *next = n->next;
>         arena_list_node_t * __arena *pprev = n->pprev;
>
>         cast_user(next);
>         cast_kern(pprev);
> -       tmp = *pprev;
> -       cast_kern(tmp);
> -       WRITE_ONCE(tmp, next);
> +       WRITE_ONCE(*pprev, next);

This looks wrong, since cast_kern() is necessary on older llvm
that don't have arena support.
On newer llvm above change should make no difference.
list_del() will still do the poisoning as it should.

I'm missing what you're trying to "fix".

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Puranjay Mohan]

On Fri, Oct 17, 2025 at 8:35 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-10-17 at 14:17 +0000, Puranjay Mohan wrote:
> > The __list_del fuction doesn't set the previous node's next pointer to
> > the next node of the node to be deleted. It just updates the local variable
> > and not the actual pointer in the previous node.
> >
> > The test was passing up till now because the bpf code is doing bpf_free()
> > after list_del and therfore reading head->first from the userspace will
> > read all zeroes. But after arena_list_del() is finished, head->first should
> > point to NULL;
> >
> > If you remove the bpf_free() call in arena_list_del(), the test will start
> > crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> > head->first and segfault.
>
> I tried commenting out bpf_free() in arena_list_del() but the test
> passes for me even w/o this patch.  Is there a way to modify the test
> case, so that logic of the list_del() is checked more thoroughly?
>

For me after commenting bpf_free() in arena_list_del() I get:

[root@localhost bpf]# ./test_progs -a arena_list
#5       arena_list:FAIL
Caught signal #11!
Stack trace:
./test_progs(crash_handler+0x1c)[0x956fd4]
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xffff885b7820]
./test_progs[0x559f00]
./test_progs[0x55a728]
./test_progs(test_arena_list+0x28)[0x55aa7c]
./test_progs[0x957624]
./test_progs(main+0x6a0)[0x959298]
/lib64/libc.so.6(+0x30558)[0xffff87e62558]
/lib64/libc.so.6(__libc_start_main+0x9c)[0xffff87e6263c]
./test_progs(_start+0x30)[0x5522f0]

I pushed it to the CI so you can see it fail:
https://github.com/kernel-patches/bpf/actions/runs/18602175717/job/53043507792

Another thing you can do in addition to commenting bpf_free() is to also comment

//n->next = LIST_POISON1;
//n->pprev = LIST_POISON2;

and now the test will not crash but fail like:

test_arena_list_add_del:FAIL:sum of list elems after del unexpected
sum of list elems after del: actual 499500 != expected 0


This is because __list_del is a no-op, and after the poisoning logic
is commented list_del() becomes a no-op.
The list stays intact after arena_list_del() finishes.

Thanks,
Puranjay

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Puranjay Mohan]

On Fri, Oct 17, 2025 at 8:41 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Oct 17, 2025 at 7:17 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> >
> > The __list_del fuction doesn't set the previous node's next pointer to
> > the next node of the node to be deleted. It just updates the local variable
> > and not the actual pointer in the previous node.
> >
> > The test was passing up till now because the bpf code is doing bpf_free()
> > after list_del and therfore reading head->first from the userspace will
> > read all zeroes. But after arena_list_del() is finished, head->first should
> > point to NULL;
> >
> > If you remove the bpf_free() call in arena_list_del(), the test will start
> > crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> > head->first and segfault.
> >
> > Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/bpf_arena_list.h | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
> > index 85dbc3ea4da5..e16fa7d95fcf 100644
> > --- a/tools/testing/selftests/bpf/bpf_arena_list.h
> > +++ b/tools/testing/selftests/bpf/bpf_arena_list.h
> > @@ -64,14 +64,12 @@ static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
> >
> >  static inline void __list_del(arena_list_node_t *n)
> >  {
> > -       arena_list_node_t *next = n->next, *tmp;
> > +       arena_list_node_t *next = n->next;
> >         arena_list_node_t * __arena *pprev = n->pprev;
> >
> >         cast_user(next);
> >         cast_kern(pprev);
> > -       tmp = *pprev;
> > -       cast_kern(tmp);
> > -       WRITE_ONCE(tmp, next);
> > +       WRITE_ONCE(*pprev, next);
>
> This looks wrong, since cast_kern() is necessary on older llvm
> that don't have arena support.
> On newer llvm above change should make no difference.
> list_del() will still do the poisoning as it should.
>
> I'm missing what you're trying to "fix".


The logic is broken, __list_del() is not removing the node.

For a second assume, everything is in kernel code and not arena,
I am also removing WRITE_ONCE for the sake of explanation:

so, the lines will become (removing all arena magic):


arena_list_node_t *next = n->next, *tmp;
arena_list_node_t **pprev = n->pprev;

tmp = *pprev;
tmp = next;

[...]

tmp is a local variable and setting it to next doesn't modify the list.

What we want to do is:

*pprev = next;

see: https://github.com/kernel-patches/bpf/actions/runs/18602175717/job/53043507792


Thanks,
Puranjay

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Yonghong Song]

On 10/17/25 12:09 PM, Puranjay Mohan wrote:
> On Fri, Oct 17, 2025 at 8:35 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>> On Fri, 2025-10-17 at 14:17 +0000, Puranjay Mohan wrote:
>>> The __list_del fuction doesn't set the previous node's next pointer to
>>> the next node of the node to be deleted. It just updates the local variable
>>> and not the actual pointer in the previous node.
>>>
>>> The test was passing up till now because the bpf code is doing bpf_free()
>>> after list_del and therfore reading head->first from the userspace will
>>> read all zeroes. But after arena_list_del() is finished, head->first should
>>> point to NULL;
>>>
>>> If you remove the bpf_free() call in arena_list_del(), the test will start
>>> crashing because now the userpsace will read 0x100 (LIST_POISON1) in
>>> head->first and segfault.
>> I tried commenting out bpf_free() in arena_list_del() but the test
>> passes for me even w/o this patch.  Is there a way to modify the test
>> case, so that logic of the list_del() is checked more thoroughly?
>>
> For me after commenting bpf_free() in arena_list_del() I get:
>
> [root@localhost bpf]# ./test_progs -a arena_list
> #5       arena_list:FAIL
> Caught signal #11!
> Stack trace:
> ./test_progs(crash_handler+0x1c)[0x956fd4]
> linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xffff885b7820]
> ./test_progs[0x559f00]
> ./test_progs[0x55a728]
> ./test_progs(test_arena_list+0x28)[0x55aa7c]
> ./test_progs[0x957624]
> ./test_progs(main+0x6a0)[0x959298]
> /lib64/libc.so.6(+0x30558)[0xffff87e62558]
> /lib64/libc.so.6(__libc_start_main+0x9c)[0xffff87e6263c]
> ./test_progs(_start+0x30)[0x5522f0]
>
> I pushed it to the CI so you can see it fail:
> https://github.com/kernel-patches/bpf/actions/runs/18602175717/job/53043507792

Just FYI, I tried and can reproduce the above crash with bpf_free() removed:

[root@arch-fb-vm1 bpf]# ./test_progs -t arena_list
#5       arena_list:FAIL
Caught signal #11!
Stack trace:
./test_progs(crash_handler+0x1f)[0x55e64108298f]
/usr/lib/libc.so.6(+0x3e710)[0x7fc0ac8f0710]
./test_progs(+0x2ea4a)[0x55e640c71a4a]
./test_progs(+0x2e5fe)[0x55e640c715fe]
./test_progs(test_arena_list+0x20)[0x55e640c70d60]
./test_progs(+0x441488)[0x55e641084488]
./test_progs(main+0x579)[0x55e641083419]
/usr/lib/libc.so.6(+0x27cd0)[0x7fc0ac8d9cd0]
/usr/lib/libc.so.6(__libc_start_main+0x8a)[0x7fc0ac8d9d8a]
./te[  161.118394] test_progs[2006]: segfault at 100 ip 000055e640c71a4a sp 00007ffeaa9775e0 error 4 in test_progs[2ea4a,55e6)
st_progs(_start+[  161.123449] Code: 8b 45 80 48 89 45 d0 48 8b 45 d0 48 89 45 f0 48 c7 45 e0 00 00 00 00 31 c0 48 83 7d f0 05
0x25)[0x55e640c697b5]
Segmentation fault (core dumped)

>
> Another thing you can do in addition to commenting bpf_free() is to also comment
>
> //n->next = LIST_POISON1;
> //n->pprev = LIST_POISON2;
>
> and now the test will not crash but fail like:
>
> test_arena_list_add_del:FAIL:sum of list elems after del unexpected
> sum of list elems after del: actual 499500 != expected 0
>
>
> This is because __list_del is a no-op, and after the poisoning logic
> is commented list_del() becomes a no-op.
> The list stays intact after arena_list_del() finishes.
>
> Thanks,
> Puranjay
>

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Eduard Zingerman]

On Fri, 2025-10-17 at 21:09 +0200, Puranjay Mohan wrote:

[...]

> [root@localhost bpf]# ./test_progs -a arena_list
> #5       arena_list:FAIL
> Caught signal #11!
> Stack trace:
> ./test_progs(crash_handler+0x1c)[0x956fd4]
> linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xffff885b7820]
> ./test_progs[0x559f00]
> ./test_progs[0x55a728]
> ./test_progs(test_arena_list+0x28)[0x55aa7c]
> ./test_progs[0x957624]
> ./test_progs(main+0x6a0)[0x959298]
> /lib64/libc.so.6(+0x30558)[0xffff87e62558]
> /lib64/libc.so.6(__libc_start_main+0x9c)[0xffff87e6263c]
> ./test_progs(_start+0x30)[0x5522f0]
> 
> I pushed it to the CI so you can see it fail:
> https://github.com/kernel-patches/bpf/actions/runs/18602175717/job/53043507792
> 
> Another thing you can do in addition to commenting bpf_free() is to also comment
> 
> //n->next = LIST_POISON1;
> //n->pprev = LIST_POISON2;
> 
> and now the test will not crash but fail like:
> 
> test_arena_list_add_del:FAIL:sum of list elems after del unexpected
> sum of list elems after del: actual 499500 != expected 0

Ok, that was an error on my side. The test fails after clean rebuild,
sorry for the noise.

But my point is that if we care about list_del() correctness, the test
should be modified so that the bug is visible w/o commenting anything.

> This is because __list_del is a no-op, and after the poisoning logic
> is commented list_del() becomes a no-op.
> The list stays intact after arena_list_del() finishes.

Yes, I understand what you are fixing.

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[Alexei Starovoitov]

On Fri, Oct 17, 2025 at 12:18 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Fri, Oct 17, 2025 at 8:41 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Oct 17, 2025 at 7:17 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> > >
> > > The __list_del fuction doesn't set the previous node's next pointer to
> > > the next node of the node to be deleted. It just updates the local variable
> > > and not the actual pointer in the previous node.
> > >
> > > The test was passing up till now because the bpf code is doing bpf_free()
> > > after list_del and therfore reading head->first from the userspace will
> > > read all zeroes. But after arena_list_del() is finished, head->first should
> > > point to NULL;
> > >
> > > If you remove the bpf_free() call in arena_list_del(), the test will start
> > > crashing because now the userpsace will read 0x100 (LIST_POISON1) in
> > > head->first and segfault.
> > >
> > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> > > ---
> > >  tools/testing/selftests/bpf/bpf_arena_list.h | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/bpf_arena_list.h b/tools/testing/selftests/bpf/bpf_arena_list.h
> > > index 85dbc3ea4da5..e16fa7d95fcf 100644
> > > --- a/tools/testing/selftests/bpf/bpf_arena_list.h
> > > +++ b/tools/testing/selftests/bpf/bpf_arena_list.h
> > > @@ -64,14 +64,12 @@ static inline void list_add_head(arena_list_node_t *n, arena_list_head_t *h)
> > >
> > >  static inline void __list_del(arena_list_node_t *n)
> > >  {
> > > -       arena_list_node_t *next = n->next, *tmp;
> > > +       arena_list_node_t *next = n->next;
> > >         arena_list_node_t * __arena *pprev = n->pprev;
> > >
> > >         cast_user(next);
> > >         cast_kern(pprev);
> > > -       tmp = *pprev;
> > > -       cast_kern(tmp);
> > > -       WRITE_ONCE(tmp, next);
> > > +       WRITE_ONCE(*pprev, next);
> >
> > This looks wrong, since cast_kern() is necessary on older llvm
> > that don't have arena support.
> > On newer llvm above change should make no difference.
> > list_del() will still do the poisoning as it should.
> >
> > I'm missing what you're trying to "fix".
>
>
> The logic is broken, __list_del() is not removing the node.
>
> For a second assume, everything is in kernel code and not arena,
> I am also removing WRITE_ONCE for the sake of explanation:
>
> so, the lines will become (removing all arena magic):
>
>
> arena_list_node_t *next = n->next, *tmp;
> arena_list_node_t **pprev = n->pprev;
>
> tmp = *pprev;
> tmp = next;
>
> [...]
>
> tmp is a local variable and setting it to next doesn't modify the list.

D'oh. Not sure what I was thinking.

Re: [PATCH bpf-next] selftests/bpf: fix list_del() in arena list

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri, 17 Oct 2025 14:17:25 +0000 you wrote:
> The __list_del fuction doesn't set the previous node's next pointer to
> the next node of the node to be deleted. It just updates the local variable
> and not the actual pointer in the previous node.
> 
> The test was passing up till now because the bpf code is doing bpf_free()
> after list_del and therfore reading head->first from the userspace will
> read all zeroes. But after arena_list_del() is finished, head->first should
> point to NULL;
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: fix list_del() in arena list
    https://git.kernel.org/bpf/bpf-next/c/7361c864852f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html