* [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue
@ 2024-06-08 9:29 Florian Lehner
2024-06-10 5:32 ` Yonghong Song
2024-06-11 19:14 ` Alexei Starovoitov
0 siblings, 2 replies; 3+ messages in thread
From: Florian Lehner @ 2024-06-08 9:29 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue
2024-06-08 9:29 [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue Florian Lehner
@ 2024-06-10 5:32 ` Yonghong Song
2024-06-11 19:14 ` Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Yonghong Song @ 2024-06-10 5:32 UTC (permalink / raw)
To: Florian Lehner, bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, john.fastabend,
kpsingh, sdf, haoluo, jolsa
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 */
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue
2024-06-08 9:29 [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue Florian Lehner
2024-06-10 5:32 ` Yonghong Song
@ 2024-06-11 19:14 ` Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2024-06-11 19:14 UTC (permalink / raw)
To: Florian Lehner
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-11 19:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-08 9:29 [PATCH bpf-next] bpf: Return EINVAL instead of NULL for map_lookup_elem of queue Florian Lehner
2024-06-10 5:32 ` Yonghong Song
2024-06-11 19:14 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox