[PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue

[PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue

[Florian Lehner]

Programs should use map_peek_elem over map_lookup_elem for queues. NULL is
also not a valid queue return nor a proper error, that could be handled.

Signed-off-by: Florian Lehner <dev@der-flo.net>
---
 kernel/bpf/queue_stack_maps.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/queue_stack_maps.c b/kernel/bpf/queue_stack_maps.c
index d869f51ea93a..85bead55024d 100644
--- a/kernel/bpf/queue_stack_maps.c
+++ b/kernel/bpf/queue_stack_maps.c
@@ -234,7 +234,8 @@ static long queue_stack_map_push_elem(struct bpf_map *map, void *value,
 /* Called from syscall or from eBPF program */
 static void *queue_stack_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	/* The eBPF program should use map_peek_elem instead */
+	return ERR_PTR(-EINVAL);
 }
 
 /* Called from syscall or from eBPF program */
-- 
2.45.2

Re: [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue

[Yonghong Song]

On 6/8/24 2:29 AM, Florian Lehner wrote:
> Programs should use map_peek_elem over map_lookup_elem for queues. NULL is
> also not a valid queue return nor a proper error, that could be handled.
>
> Signed-off-by: Florian Lehner <dev@der-flo.net>

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

> ---
>   kernel/bpf/queue_stack_maps.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/queue_stack_maps.c b/kernel/bpf/queue_stack_maps.c
> index d869f51ea93a..85bead55024d 100644
> --- a/kernel/bpf/queue_stack_maps.c
> +++ b/kernel/bpf/queue_stack_maps.c
> @@ -234,7 +234,8 @@ static long queue_stack_map_push_elem(struct bpf_map *map, void *value,
>   /* Called from syscall or from eBPF program */
>   static void *queue_stack_map_lookup_elem(struct bpf_map *map, void *key)
>   {
> -	return NULL;
> +	/* The eBPF program should use map_peek_elem instead */
> +	return ERR_PTR(-EINVAL);
>   }
>   
>   /* Called from syscall or from eBPF program */

Re: [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue

[Alexei Starovoitov]

On Sat, Jun 8, 2024 at 2:29 AM Florian Lehner <dev@der-flo.net> wrote:
>
> Programs should use map_peek_elem over map_lookup_elem for queues. NULL is
> also not a valid queue return nor a proper error, that could be handled.
>
> Signed-off-by: Florian Lehner <dev@der-flo.net>
> ---
>  kernel/bpf/queue_stack_maps.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/queue_stack_maps.c b/kernel/bpf/queue_stack_maps.c
> index d869f51ea93a..85bead55024d 100644
> --- a/kernel/bpf/queue_stack_maps.c
> +++ b/kernel/bpf/queue_stack_maps.c
> @@ -234,7 +234,8 @@ static long queue_stack_map_push_elem(struct bpf_map *map, void *value,
>  /* Called from syscall or from eBPF program */
>  static void *queue_stack_map_lookup_elem(struct bpf_map *map, void *key)
>  {
> -       return NULL;
> +       /* The eBPF program should use map_peek_elem instead */
> +       return ERR_PTR(-EINVAL);

The commit log, the code change and comment are highly misleading.
bpf prog cannot call this function due to the verifier restrictions.
bpf syscall cannot reach this code path either.
This is effectively a dead code.
But if there is a verifier bug or obscure sequence of events
and bpf prog manages to call this function then it must return NULL.
ERR_PTR(-EINVAL) will look like a valid addr to a prog and it will crash.

pw-bot: cr