* [PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function
@ 2022-05-12 14:17 Wan Jiabing
2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Wan Jiabing @ 2022-05-12 14:17 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel
Cc: 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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach 2022-05-12 14:17 [PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function Wan Jiabing @ 2022-05-12 14:17 ` Wan Jiabing 2022-05-12 16:40 ` Song Liu 2022-05-12 17:05 ` David Vernet 2022-05-12 14:17 ` [PATCH 2/3] bpf: simplify if-if to if " Wan Jiabing 2022-05-12 14:17 ` [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user Wan Jiabing 2 siblings, 2 replies; 8+ messages in thread From: Wan Jiabing @ 2022-05-12 14:17 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach 2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing @ 2022-05-12 16:40 ` Song Liu 2022-05-12 17:05 ` David Vernet 1 sibling, 0 replies; 8+ messages in thread From: Song Liu @ 2022-05-12 16:40 UTC (permalink / raw) To: Wan Jiabing Cc: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau, Yonghong Song, John Fastabend, KP Singh, Networking, bpf, Linux Kernel Mailing List > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach 2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing 2022-05-12 16:40 ` Song Liu @ 2022-05-12 17:05 ` David Vernet 1 sibling, 0 replies; 8+ messages in thread From: David Vernet @ 2022-05-12 17:05 UTC (permalink / raw) To: Wan Jiabing Cc: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, netdev, bpf, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] bpf: simplify if-if to if in bpf_kprobe_multi_link_attach 2022-05-12 14:17 [PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function Wan Jiabing 2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing @ 2022-05-12 14:17 ` Wan Jiabing 2022-05-12 16:44 ` Song Liu 2022-05-12 14:17 ` [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user Wan Jiabing 2 siblings, 1 reply; 8+ messages in thread From: Wan Jiabing @ 2022-05-12 14:17 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] bpf: simplify if-if to if in bpf_kprobe_multi_link_attach 2022-05-12 14:17 ` [PATCH 2/3] bpf: simplify if-if to if " Wan Jiabing @ 2022-05-12 16:44 ` Song Liu 0 siblings, 0 replies; 8+ messages in thread From: Song Liu @ 2022-05-12 16:44 UTC (permalink / raw) To: Wan Jiabing Cc: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau, Yonghong Song, John Fastabend, KP Singh, Networking, bpf, Linux Kernel Mailing List > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user 2022-05-12 14:17 [PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function Wan Jiabing 2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing 2022-05-12 14:17 ` [PATCH 2/3] bpf: simplify if-if to if " Wan Jiabing @ 2022-05-12 14:17 ` Wan Jiabing 2022-05-12 16:45 ` Song Liu 2 siblings, 1 reply; 8+ messages in thread From: Wan Jiabing @ 2022-05-12 14:17 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user 2022-05-12 14:17 ` [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user Wan Jiabing @ 2022-05-12 16:45 ` Song Liu 0 siblings, 0 replies; 8+ messages in thread From: Song Liu @ 2022-05-12 16:45 UTC (permalink / raw) To: Wan Jiabing Cc: Steven Rostedt, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau, Yonghong Song, John Fastabend, KP Singh, Networking, bpf, Linux Kernel Mailing List > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-12 17:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-12 14:17 [PATCH 0/3] bpf: optimize the bpf_kprobe_multi_link_attach function Wan Jiabing 2022-05-12 14:17 ` [PATCH 1/3] bpf: use 'error_xxx' tags in bpf_kprobe_multi_link_attach Wan Jiabing 2022-05-12 16:40 ` Song Liu 2022-05-12 17:05 ` David Vernet 2022-05-12 14:17 ` [PATCH 2/3] bpf: simplify if-if to if " Wan Jiabing 2022-05-12 16:44 ` Song Liu 2022-05-12 14:17 ` [PATCH 3/3] bpf: use vmemdup_user instead of kvmalloc and copy_from_user Wan Jiabing 2022-05-12 16:45 ` Song Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).