[PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function

[PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function

[Wan Jiabing]

This patch series tries to optimize the bpf_kporbe_multi_link_attach
in bpf_trace.c

Firstly, there is only one 'error' tag to handle error code. But in one
'error' tag, there are three 'free' functions which is not efficient. So
I split this one tag to three tags to make it clear.

Secondly, I simplify double 'if' statements to one statement.

Finally, coccicheck reports an opportunity for vmemdup_user. So I use
vmemdup_user to make code cleaner.

Wan Jiabing (3):
  bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach
  bpf: simplify if-if to if in bpf_kprobe_multi_link_attach
  bpf: use vmemdup_user instead of kvmalloc and copy_from_user

 kernel/trace/bpf_trace.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

-- 
2.35.1

[PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach

[Wan Jiabing]

Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error
handling more efficient.

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
 kernel/trace/bpf_trace.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 2eaac094caf8..3a8b69ef9a0d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2467,20 +2467,20 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (uaddrs) {
 		if (copy_from_user(addrs, uaddrs, size)) {
 			err = -EFAULT;
-			goto error;
+			goto error_addrs;
 		}
 	} else {
 		struct user_syms us;
 
 		err = copy_user_syms(&us, usyms, cnt);
 		if (err)
-			goto error;
+			goto error_addrs;
 
 		sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
 		err = ftrace_lookup_symbols(us.syms, cnt, addrs);
 		free_user_syms(&us);
 		if (err)
-			goto error;
+			goto error_addrs;
 	}
 
 	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
@@ -2488,18 +2488,18 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		cookies = kvmalloc(size, GFP_KERNEL);
 		if (!cookies) {
 			err = -ENOMEM;
-			goto error;
+			goto error_addrs;
 		}
 		if (copy_from_user(cookies, ucookies, size)) {
 			err = -EFAULT;
-			goto error;
+			goto error_cookies;
 		}
 	}
 
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link) {
 		err = -ENOMEM;
-		goto error;
+		goto error_cookies;
 	}
 
 	bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI,
@@ -2507,7 +2507,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err)
-		goto error;
+		goto error_link;
 
 	if (flags & BPF_F_KPROBE_MULTI_RETURN)
 		link->fp.exit_handler = kprobe_multi_link_handler;
@@ -2539,10 +2539,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 
 	return bpf_link_settle(&link_primer);
 
-error:
+error_link:
 	kfree(link);
-	kvfree(addrs);
+error_cookies:
 	kvfree(cookies);
+error_addrs:
+	kvfree(addrs);
 	return err;
 }
 #else /* !CONFIG_FPROBE */
-- 
2.35.1

[PATCH 2/3] bpf: simplify if-if to if in bpf_kprobe_multi_link_attach

[Wan Jiabing]

Simplify double 'if' statements to one 'if' statement.

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
 kernel/trace/bpf_trace.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3a8b69ef9a0d..1b0db8f78dc8 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2464,11 +2464,9 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (!addrs)
 		return -ENOMEM;
 
-	if (uaddrs) {
-		if (copy_from_user(addrs, uaddrs, size)) {
-			err = -EFAULT;
-			goto error_addrs;
-		}
+	if (uaddrs && copy_from_user(addrs, uaddrs, size)) {
+		err = -EFAULT;
+		goto error_addrs;
 	} else {
 		struct user_syms us;
 
-- 
2.35.1

[PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user

[Wan Jiabing]

Fix following coccicheck warning:
./kernel/trace/bpf_trace.c:2488:12-20: WARNING opportunity for vmemdup_user

Use vmemdup_user instead of kvmalloc and copy_from_user.

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
 kernel/trace/bpf_trace.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1b0db8f78dc8..48fc97a6db50 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2483,15 +2483,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 
 	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
 	if (ucookies) {
-		cookies = kvmalloc(size, GFP_KERNEL);
-		if (!cookies) {
-			err = -ENOMEM;
+		cookies = vmemdup_user(ucookies, size);
+		if (IS_ERR(cookies)) {
+			err = PTR_ERR(cookies);
 			goto error_addrs;
 		}
-		if (copy_from_user(cookies, ucookies, size)) {
-			err = -EFAULT;
-			goto error_cookies;
-		}
 	}
 
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
-- 
2.35.1

Re: [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach

[Song Liu]

> On May 12, 2022, at 7:17 AM, Wan Jiabing <wanjiabing@vivo.com> wrote:
> 
> Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error
> handling more efficient.
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> kernel/trace/bpf_trace.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)

I seriously think current version is simpler. 

Song

Re: [PATCH 2/3] bpf: simplify if-if to if in bpf_kprobe_multi_link_attach

[Song Liu]

> On May 12, 2022, at 7:17 AM, Wan Jiabing <wanjiabing@vivo.com> wrote:
> 
> Simplify double 'if' statements to one 'if' statement.
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> kernel/trace/bpf_trace.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 3a8b69ef9a0d..1b0db8f78dc8 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2464,11 +2464,9 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> 	if (!addrs)
> 		return -ENOMEM;
> 
> -	if (uaddrs) {
> -		if (copy_from_user(addrs, uaddrs, size)) {
> -			err = -EFAULT;
> -			goto error_addrs;
> -		}
> +	if (uaddrs && copy_from_user(addrs, uaddrs, size)) {
> +		err = -EFAULT;
> +		goto error_addrs;
> 	} else {
> 		struct user_syms us;

This changed the behavior, no?

For uaddrs != NULL and copy_from_user() == 0 case, we now going into
else clause. Did I misread anything?

Song

Re: [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user

[Song Liu]

> On May 12, 2022, at 7:17 AM, Wan Jiabing <wanjiabing@vivo.com> wrote:
> 
> Fix following coccicheck warning:
> ./kernel/trace/bpf_trace.c:2488:12-20: WARNING opportunity for vmemdup_user
> 
> Use vmemdup_user instead of kvmalloc and copy_from_user.
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> kernel/trace/bpf_trace.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 1b0db8f78dc8..48fc97a6db50 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2483,15 +2483,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> 
> 	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> 	if (ucookies) {
> -		cookies = kvmalloc(size, GFP_KERNEL);
> -		if (!cookies) {
> -			err = -ENOMEM;
> +		cookies = vmemdup_user(ucookies, size);

vmemdup_user() uses GFP_USER, so this is a behavior change. 

Song

> +		if (IS_ERR(cookies)) {
> +			err = PTR_ERR(cookies);
> 			goto error_addrs;
> 		}
> -		if (copy_from_user(cookies, ucookies, size)) {
> -			err = -EFAULT;
> -			goto error_cookies;
> -		}
> 	}
> 
> 	link = kzalloc(sizeof(*link), GFP_KERNEL);
> -- 
> 2.35.1
> 

Re: [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach

[David Vernet]

On Thu, May 12, 2022 at 10:17:08PM +0800, Wan Jiabing wrote:
> Use 'error_addrs', 'error_cookies' and 'error_link' tags to make error
> handling more efficient.

Can you add a bit more context to this commit summary? The added goto
labels aren't what make the code more performant, it's the avoidance of
unnecessary free calls on NULL pointers that (supposedly) does.

> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
>  kernel/trace/bpf_trace.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 2eaac094caf8..3a8b69ef9a0d 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2467,20 +2467,20 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	if (uaddrs) {
>  		if (copy_from_user(addrs, uaddrs, size)) {
>  			err = -EFAULT;
> -			goto error;
> +			goto error_addrs;
>  		}
>  	} else {
>  		struct user_syms us;
>  
>  		err = copy_user_syms(&us, usyms, cnt);
>  		if (err)
> -			goto error;
> +			goto error_addrs;
>  
>  		sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
>  		err = ftrace_lookup_symbols(us.syms, cnt, addrs);
>  		free_user_syms(&us);
>  		if (err)
> -			goto error;
> +			goto error_addrs;
>  	}
>  
>  	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> @@ -2488,18 +2488,18 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  		cookies = kvmalloc(size, GFP_KERNEL);
>  		if (!cookies) {
>  			err = -ENOMEM;
> -			goto error;
> +			goto error_addrs;
>  		}
>  		if (copy_from_user(cookies, ucookies, size)) {
>  			err = -EFAULT;
> -			goto error;
> +			goto error_cookies;
>  		}
>  	}
>  
>  	link = kzalloc(sizeof(*link), GFP_KERNEL);
>  	if (!link) {
>  		err = -ENOMEM;
> -		goto error;
> +		goto error_cookies;
>  	}
>  
>  	bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI,
> @@ -2507,7 +2507,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  
>  	err = bpf_link_prime(&link->link, &link_primer);
>  	if (err)
> -		goto error;
> +		goto error_link;
>  
>  	if (flags & BPF_F_KPROBE_MULTI_RETURN)
>  		link->fp.exit_handler = kprobe_multi_link_handler;
> @@ -2539,10 +2539,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  
>  	return bpf_link_settle(&link_primer);
>  
> -error:
> +error_link:
>  	kfree(link);
> -	kvfree(addrs);
> +error_cookies:
>  	kvfree(cookies);
> +error_addrs:
> +	kvfree(addrs);
>  	return err;
>  }
>  #else /* !CONFIG_FPROBE */
> -- 
> 2.35.1
> 

Could you clarify what performance gains you observed from doing this? I
wouldn't have expected avoiding a couple of calls and NULL checks to have a
measurable impact on performance, and I'm wondering whether the complexity
from having multiple goto labels is really worth any supposed performance
gains.

Thanks,
David