[-next,2/5] bpf: Remove map_push_elem callbacks with -EOPNOTSUPP

Commit Message

Xiaomeng Zhang Feb. 17, 2025, 1:41 a.m. UTC

Remove redundant map_push_elem callbacks with return -EOPNOTSUPP. We can
directly return -EOPNOTSUPP when calling the unimplemented callbacks.

Signed-off-by: Xiaomeng Zhang <[e-mail]>
---
 kernel/bpf/arena.c   | 6 ------
 kernel/bpf/helpers.c | 5 ++++-
 kernel/bpf/syscall.c | 5 ++++-
 3 files changed, 8 insertions(+), 8 deletions(-)

Comments

Hou Tao Feb. 17, 2025, 2:27 a.m. UTC | #1

On 2/17/2025 9:41 AM, Xiaomeng Zhang wrote:
> Remove redundant map_push_elem callbacks with return -EOPNOTSUPP. We can
> directly return -EOPNOTSUPP when calling the unimplemented callbacks.
>
>
>  BPF_CALL_3(bpf_map_push_elem, struct bpf_map *, map, void *, value, u64, flags)
>  {
> -	return map->ops->map_push_elem(map, value, flags);
> +	if (map->ops->map_push_elem)
> +		return map->ops->map_push_elem(map, value, flags);
> +	else
> +		return -EOPNOTSUPP;
>  }

Similar with the previous patch, the modifications in both
bpf_map_push_elem() and bpf_map_update_valu() are unnecessary.
>  
>  const struct bpf_func_proto bpf_map_push_elem_proto = {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index e6e859f71c5d..79a118ea9270 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -281,7 +281,10 @@ static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
>  	} else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>  		   map->map_type == BPF_MAP_TYPE_STACK ||
>  		   map->map_type == BPF_MAP_TYPE_BLOOM_FILTER) {
> -		err = map->ops->map_push_elem(map, value, flags);
> +		if (map->ops->map_push_elem)
> +			err = map->ops->map_push_elem(map, value, flags);
> +		else
> +			err = -EOPNOTSUPP;
>  	} else {
>  		err = bpf_obj_pin_uptrs(map->record, value);
>  		if (!err) {

Xiaomeng Zhang Feb. 17, 2025, 12:45 p.m. UTC | #2

Hi,
Although when the BPF program invokes the bpf_map_peek_elem helper, the verifier intercepts it and ensures that this helper is only available for specific map types through check_map_func_compatibility(), there is no such verifier interception when this function is called from the syscall side.

Therefore, it is necessary to include appropriate checks in the syscall implementation to handle cases where map->ops->map_peek_elem may not be supported by certain map types. Without these checks, calling bpf_map_peek_elem through syscalls could lead to undefined behavior or errors if the map does not support this operation.

-----邮件原件-----
发件人: Hou Tao <[e-mail]> 
发送时间: 2025年2月17日 10:28
收件人: zhangxiaomeng (A) <[e-mail]>; [e-mail]
抄送: Alexei Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>; Andrii Nakryiko <[e-mail]>; Martin KaFai Lau <[e-mail]>; Eduard Zingerman <[e-mail]>; Song Liu <[e-mail]>; Yonghong Song <[e-mail]>; John Fastabend <[e-mail]>; KP Singh <[e-mail]>; Stanislav Fomichev <[e-mail]>; Hao Luo <[e-mail]>; Jiri Olsa <[e-mail]>
主题: Re: [PATCH -next 2/5] bpf: Remove map_push_elem callbacks with -EOPNOTSUPP



On 2/17/2025 9:41 AM, Xiaomeng Zhang wrote:
> Remove redundant map_push_elem callbacks with return -EOPNOTSUPP. We 
> can directly return -EOPNOTSUPP when calling the unimplemented callbacks.
>
>
>  BPF_CALL_3(bpf_map_push_elem, struct bpf_map *, map, void *, value, 
> u64, flags)  {
> -	return map->ops->map_push_elem(map, value, flags);
> +	if (map->ops->map_push_elem)
> +		return map->ops->map_push_elem(map, value, flags);
> +	else
> +		return -EOPNOTSUPP;
>  }

Similar with the previous patch, the modifications in both
bpf_map_push_elem() and bpf_map_update_valu() are unnecessary.
>  
>  const struct bpf_func_proto bpf_map_push_elem_proto = { diff --git 
> a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 
> e6e859f71c5d..79a118ea9270 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -281,7 +281,10 @@ static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
>  	} else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>  		   map->map_type == BPF_MAP_TYPE_STACK ||
>  		   map->map_type == BPF_MAP_TYPE_BLOOM_FILTER) {
> -		err = map->ops->map_push_elem(map, value, flags);
> +		if (map->ops->map_push_elem)
> +			err = map->ops->map_push_elem(map, value, flags);
> +		else
> +			err = -EOPNOTSUPP;
>  	} else {
>  		err = bpf_obj_pin_uptrs(map->record, value);
>  		if (!err) {

Patch

diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
index 0aaefa5d6b09..7ee98aeccf3c 100644
--- a/kernel/bpf/arena.c
+++ b/kernel/bpf/arena.c
@@ -62,11 +62,6 @@  u64 bpf_arena_get_user_vm_start(struct bpf_arena *arena)
 	return arena ? arena->user_vm_start : 0;
 }
 
-static long arena_map_push_elem(struct bpf_map *map, void *value, u64 flags)
-{
-	return -EOPNOTSUPP;
-}
-
 static long arena_map_pop_elem(struct bpf_map *map, void *value)
 {
 	return -EOPNOTSUPP;
@@ -398,7 +393,6 @@  const struct bpf_map_ops arena_map_ops = {
 	.map_mmap = arena_map_mmap,
 	.map_get_unmapped_area = arena_get_unmapped_area,
 	.map_get_next_key = arena_map_get_next_key,
-	.map_push_elem = arena_map_push_elem,
 	.map_pop_elem = arena_map_pop_elem,
 	.map_lookup_elem = arena_map_lookup_elem,
 	.map_update_elem = arena_map_update_elem,
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 0f429171de6d..000409c8308a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -88,7 +88,10 @@  const struct bpf_func_proto bpf_map_delete_elem_proto = {
 
 BPF_CALL_3(bpf_map_push_elem, struct bpf_map *, map, void *, value, u64, flags)
 {
-	return map->ops->map_push_elem(map, value, flags);
+	if (map->ops->map_push_elem)
+		return map->ops->map_push_elem(map, value, flags);
+	else
+		return -EOPNOTSUPP;
 }
 
 const struct bpf_func_proto bpf_map_push_elem_proto = {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e6e859f71c5d..79a118ea9270 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -281,7 +281,10 @@  static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
 	} else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
 		   map->map_type == BPF_MAP_TYPE_STACK ||
 		   map->map_type == BPF_MAP_TYPE_BLOOM_FILTER) {
-		err = map->ops->map_push_elem(map, value, flags);
+		if (map->ops->map_push_elem)
+			err = map->ops->map_push_elem(map, value, flags);
+		else
+			err = -EOPNOTSUPP;
 	} else {
 		err = bpf_obj_pin_uptrs(map->record, value);
 		if (!err) {