[bpf-next] bpf: Check IS_ERR for the bpf_map_get() return value

Commit Message

Martin KaFai Lau March 24, 2023, 6:42 p.m. UTC

From: Martin KaFai Lau <[e-mail]>

This patch fixes a mistake in checking NULL instead of
checking IS_ERR for the bpf_map_get() return value.

It also fixes the return value in link_update_map() from -EINVAL
to PTR_ERR(*_map).

Reported-by: [e-mail]
Fixes: 68b04864ca42 ("bpf: Create links for BPF struct_ops maps.")
Fixes: aef56f2e918b ("bpf: Update the struct_ops of a bpf_link.")
Signed-off-by: Martin KaFai Lau <[e-mail]>
---
 kernel/bpf/bpf_struct_ops.c | 4 ++--
 kernel/bpf/syscall.c        | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Kui-Feng Lee March 24, 2023, 7:20 p.m. UTC | #1

This fix makes sense to me.
Thank you for fixing this.

Acked-by: Kui-Feng Lee <[e-mail]>


On 3/24/23 11:42, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <[e-mail]>
> 
> This patch fixes a mistake in checking NULL instead of
> checking IS_ERR for the bpf_map_get() return value.
> 
> It also fixes the return value in link_update_map() from -EINVAL
> to PTR_ERR(*_map).
> 
> Reported-by: [e-mail]
> Fixes: 68b04864ca42 ("bpf: Create links for BPF struct_ops maps.")
> Fixes: aef56f2e918b ("bpf: Update the struct_ops of a bpf_link.")
> Signed-off-by: Martin KaFai Lau <[e-mail]>
> ---
>   kernel/bpf/bpf_struct_ops.c | 4 ++--
>   kernel/bpf/syscall.c        | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 6401deca3b56..d3f0a4825fa6 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -871,8 +871,8 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>   	int err;
>   
>   	map = bpf_map_get(attr->link_create.map_fd);
> -	if (!map)
> -		return -EINVAL;
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
>   
>   	st_map = (struct bpf_struct_ops_map *)map;
>   
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index b4d758fa5981..a09597c95029 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4689,12 +4689,12 @@ static int link_update_map(struct bpf_link *link, union bpf_attr *attr)
>   
>   	new_map = bpf_map_get(attr->link_update.new_map_fd);
>   	if (IS_ERR(new_map))
> -		return -EINVAL;
> +		return PTR_ERR(new_map);
>   
>   	if (attr->link_update.flags & BPF_F_REPLACE) {
>   		old_map = bpf_map_get(attr->link_update.old_map_fd);
>   		if (IS_ERR(old_map)) {
> -			ret = -EINVAL;
> +			ret = PTR_ERR(old_map);
>   			goto out_put;
>   		}
>   	} else if (attr->link_update.old_map_fd) {

Stanislav Fomichev March 24, 2023, 7:37 p.m. UTC | #2

On 03/24, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <[e-mail]>

> This patch fixes a mistake in checking NULL instead of
> checking IS_ERR for the bpf_map_get() return value.

> It also fixes the return value in link_update_map() from -EINVAL
> to PTR_ERR(*_map).

> Reported-by: [e-mail]
> Fixes: 68b04864ca42 ("bpf: Create links for BPF struct_ops maps.")
> Fixes: aef56f2e918b ("bpf: Update the struct_ops of a bpf_link.")
> Signed-off-by: Martin KaFai Lau <[e-mail]>

Acked-by: Stanislav Fomichev <[e-mail]>

> ---
>   kernel/bpf/bpf_struct_ops.c | 4 ++--
>   kernel/bpf/syscall.c        | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)

> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 6401deca3b56..d3f0a4825fa6 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -871,8 +871,8 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>   	int err;

>   	map = bpf_map_get(attr->link_create.map_fd);
> -	if (!map)
> -		return -EINVAL;
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);

>   	st_map = (struct bpf_struct_ops_map *)map;

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index b4d758fa5981..a09597c95029 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4689,12 +4689,12 @@ static int link_update_map(struct bpf_link *link,  
> union bpf_attr *attr)

>   	new_map = bpf_map_get(attr->link_update.new_map_fd);
>   	if (IS_ERR(new_map))
> -		return -EINVAL;
> +		return PTR_ERR(new_map);

>   	if (attr->link_update.flags & BPF_F_REPLACE) {
>   		old_map = bpf_map_get(attr->link_update.old_map_fd);
>   		if (IS_ERR(old_map)) {
> -			ret = -EINVAL;
> +			ret = PTR_ERR(old_map);
>   			goto out_put;
>   		}
>   	} else if (attr->link_update.old_map_fd) {
> --
> 2.34.1

[e-mail] March 24, 2023, 7:50 p.m. UTC | #3

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

On Fri, 24 Mar 2023 11:42:41 -0700 you wrote:
> From: Martin KaFai Lau <[e-mail]>
> 
> This patch fixes a mistake in checking NULL instead of
> checking IS_ERR for the bpf_map_get() return value.
> 
> It also fixes the return value in link_update_map() from -EINVAL
> to PTR_ERR(*_map).
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Check IS_ERR for the bpf_map_get() return value
    https://git.kernel.org/bpf/bpf-next/c/55fbae05476d

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 6401deca3b56..d3f0a4825fa6 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -871,8 +871,8 @@  int bpf_struct_ops_link_create(union bpf_attr *attr)
 	int err;
 
 	map = bpf_map_get(attr->link_create.map_fd);
-	if (!map)
-		return -EINVAL;
+	if (IS_ERR(map))
+		return PTR_ERR(map);
 
 	st_map = (struct bpf_struct_ops_map *)map;
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b4d758fa5981..a09597c95029 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4689,12 +4689,12 @@  static int link_update_map(struct bpf_link *link, union bpf_attr *attr)
 
 	new_map = bpf_map_get(attr->link_update.new_map_fd);
 	if (IS_ERR(new_map))
-		return -EINVAL;
+		return PTR_ERR(new_map);
 
 	if (attr->link_update.flags & BPF_F_REPLACE) {
 		old_map = bpf_map_get(attr->link_update.old_map_fd);
 		if (IS_ERR(old_map)) {
-			ret = -EINVAL;
+			ret = PTR_ERR(old_map);
 			goto out_put;
 		}
 	} else if (attr->link_update.old_map_fd) {