[PATCH bpf] bpf: fix end-of-list detection in cgroup_storage_get_next_key()

[PATCH bpf] bpf: fix end-of-list detection in cgroup_storage_get_next_key()

[Weiming Shi]

list_next_entry() never returns NULL -- when the current element is the
last entry it wraps to the list head via container_of(). The subsequent
NULL check is therefore dead code and get_next_key() never returns
-ENOENT for the last element, instead reading storage->key from a bogus
pointer that aliases internal map fields and copying the result to
userspace.

Replace it with list_entry_is_head() so the function correctly returns
-ENOENT when there are no more entries.

Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 kernel/bpf/local_storage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index 8fca0c64f7b1..23267213a17f 100644
--- a/kernel/bpf/local_storage.c
+++ b/kernel/bpf/local_storage.c
@@ -270,7 +270,7 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
 			goto enoent;
 
 		storage = list_next_entry(storage, list_map);
-		if (!storage)
+		if (list_entry_is_head(storage, &map->list, list_map))
 			goto enoent;
 	} else {
 		storage = list_first_entry(&map->list,
-- 
2.43.0

Re: [PATCH bpf] bpf: fix end-of-list detection in cgroup_storage_get_next_key()

[sun jian]

On Thu, Apr 2, 2026 at 3:31 AM Weiming Shi <bestswngs@gmail.com> wrote:
>
> list_next_entry() never returns NULL -- when the current element is the
> last entry it wraps to the list head via container_of(). The subsequent
> NULL check is therefore dead code and get_next_key() never returns
> -ENOENT for the last element, instead reading storage->key from a bogus
> pointer that aliases internal map fields and copying the result to
> userspace.
>
> Replace it with list_entry_is_head() so the function correctly returns
> -ENOENT when there are no more entries.
>
> Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>  kernel/bpf/local_storage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 8fca0c64f7b1..23267213a17f 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -270,7 +270,7 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
>                         goto enoent;
>
>                 storage = list_next_entry(storage, list_map);
> -               if (!storage)
> +               if (list_entry_is_head(storage, &map->list, list_map))
>                         goto enoent;
>         } else {
>                 storage = list_first_entry(&map->list,
> --
> 2.43.0
>
>
Looks correct to me. It might also be worth adding a selftest for this
cornet case.

Reviewed by Sun Jian <sun.jian.kdev@gmail.com>

Re: [PATCH bpf] bpf: fix end-of-list detection in cgroup_storage_get_next_key()

[Paul Chaignon]

On Thu, Apr 02, 2026 at 08:56:06AM +0800, sun jian wrote:
> On Thu, Apr 2, 2026 at 3:31 AM Weiming Shi <bestswngs@gmail.com> wrote:
> >
> > list_next_entry() never returns NULL -- when the current element is the
> > last entry it wraps to the list head via container_of(). The subsequent
> > NULL check is therefore dead code and get_next_key() never returns
> > -ENOENT for the last element, instead reading storage->key from a bogus
> > pointer that aliases internal map fields and copying the result to
> > userspace.
> >
> > Replace it with list_entry_is_head() so the function correctly returns
> > -ENOENT when there are no more entries.
> >
> > Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>

Acked-by: Paul Chaignon <paul.chaignon@gmail.com>

> > ---
> >  kernel/bpf/local_storage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> > index 8fca0c64f7b1..23267213a17f 100644
> > --- a/kernel/bpf/local_storage.c
> > +++ b/kernel/bpf/local_storage.c
> > @@ -270,7 +270,7 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
> >                         goto enoent;
> >
> >                 storage = list_next_entry(storage, list_map);
> > -               if (!storage)
> > +               if (list_entry_is_head(storage, &map->list, list_map))
> >                         goto enoent;
> >         } else {
> >                 storage = list_first_entry(&map->list,
> > --
> > 2.43.0
> >
> >
> Looks correct to me. It might also be worth adding a selftest for this
> cornet case.

I agree, it would be good to cover this in selftests. You can use the
following diff for that:

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
index cf395715ced4..5451a43b3563 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
@@ -86,6 +86,11 @@ void test_cgroup_storage(void)
        err = SYS_NOFAIL(PING_CMD);
        ASSERT_OK(err, "sixth ping");

+       err = bpf_map__get_next_key(skel->maps.cgroup_storage, &key, &key,
+                                   sizeof(key));
+       ASSERT_ERR(err, "bpf_map__get_next_key should fail");
+       ASSERT_EQ(errno, ENOENT, "no second key");
+
 cleanup_progs:
        cgroup_storage__destroy(skel);
 cleanup_network:


> 
> Reviewed by Sun Jian <sun.jian.kdev@gmail.com>
> 

Re: [PATCH bpf] bpf: fix end-of-list detection in cgroup_storage_get_next_key()

[Weiming Shi]

On 26-04-03 15:05, Paul Chaignon wrote:
> On Thu, Apr 02, 2026 at 08:56:06AM +0800, sun jian wrote:
> > On Thu, Apr 2, 2026 at 3:31 AM Weiming Shi <bestswngs@gmail.com> wrote:
> > >
> > > list_next_entry() never returns NULL -- when the current element is the
> > > last entry it wraps to the list head via container_of(). The subsequent
> > > NULL check is therefore dead code and get_next_key() never returns
> > > -ENOENT for the last element, instead reading storage->key from a bogus
> > > pointer that aliases internal map fields and copying the result to
> > > userspace.
> > >
> > > Replace it with list_entry_is_head() so the function correctly returns
> > > -ENOENT when there are no more entries.
> > >
> > > Fixes: de9cbbaadba5 ("bpf: introduce cgroup storage maps")
> > > Reported-by: Xiang Mei <xmei5@asu.edu>
> > > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> 
> Acked-by: Paul Chaignon <paul.chaignon@gmail.com>
> 
> > > ---
> > >  kernel/bpf/local_storage.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> > > index 8fca0c64f7b1..23267213a17f 100644
> > > --- a/kernel/bpf/local_storage.c
> > > +++ b/kernel/bpf/local_storage.c
> > > @@ -270,7 +270,7 @@ static int cgroup_storage_get_next_key(struct bpf_map *_map, void *key,
> > >                         goto enoent;
> > >
> > >                 storage = list_next_entry(storage, list_map);
> > > -               if (!storage)
> > > +               if (list_entry_is_head(storage, &map->list, list_map))
> > >                         goto enoent;
> > >         } else {
> > >                 storage = list_first_entry(&map->list,
> > > --
> > > 2.43.0
> > >
> > >
> > Looks correct to me. It might also be worth adding a selftest for this
> > cornet case.
> 
> I agree, it would be good to cover this in selftests. You can use the
> following diff for that:
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
> index cf395715ced4..5451a43b3563 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
> @@ -86,6 +86,11 @@ void test_cgroup_storage(void)
>         err = SYS_NOFAIL(PING_CMD);
>         ASSERT_OK(err, "sixth ping");
> 
> +       err = bpf_map__get_next_key(skel->maps.cgroup_storage, &key, &key,
> +                                   sizeof(key));
> +       ASSERT_ERR(err, "bpf_map__get_next_key should fail");
> +       ASSERT_EQ(errno, ENOENT, "no second key");
> +
>  cleanup_progs:
>         cgroup_storage__destroy(skel);
>  cleanup_network:
> 
> 
> > 
> > Reviewed by Sun Jian <sun.jian.kdev@gmail.com>
> > 

Thanks for the your review and the selftest suggestion! I've incorporated it in v2.