Messages in this thread | | | Date | Tue, 17 May 2022 14:34:55 -0700 | Subject | Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs | From | Yonghong Song <> |
| |
On 5/17/22 1:03 PM, Jiri Olsa wrote: > On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote: >> On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote: >>> On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote: >>>> With the interface as defined, it is impossible to pass 64-bit kernel >>>> addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, >>>> which severly limits the useability of the interface, change the ABI >>>> to accept an array of u64 values instead of (kernel? user?) longs. >>>> Interestingly, the rest of the libbpf infrastructure uses 64-bit values >>>> for kallsyms addresses already, so this patch also eliminates >>>> the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb(). >>> >>> so the problem is when we have 32bit user sace on 64bit kernel right? >>> >>> I think we should keep addrs as longs in uapi and have kernel to figure out >>> if it needs to read u32 or u64, like you did for symbols in previous patch >> >> No, it's not possible here, as addrs are kernel addrs and not user space >> addrs, so user space has to explicitly pass 64-bit addresses on 64-bit >> kernels (or have a notion whether it is running on a 64-bit >> or 32-bit kernel, and form the passed array accordingly, which is against >> the idea of compat layer that tries to abstract it out). > > hum :-\ I'll need to check on compat layer.. there must > be some other code doing this already somewhere, right?
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
> jirka > >> >>> we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes >>> 64bit user space pointers >>> >>> would be gret if we could have selftest for this >>> >>> thanks, >>> jirka >>> >>>> >>>> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") >>>> Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") >>>> Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") >>>> Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") >>>> Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") >>>> Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") >>>> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> >>>> --- >>>> kernel/trace/bpf_trace.c | 25 ++++++++++++++++++---- >>>> tools/lib/bpf/bpf.h | 2 +- >>>> tools/lib/bpf/libbpf.c | 8 +++---- >>>> tools/lib/bpf/libbpf.h | 2 +- >>>> .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- >>>> .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- >>>> 6 files changed, 32 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>>> index 9d3028a..30a15b3 100644 >>>> --- a/kernel/trace/bpf_trace.c >>>> +++ b/kernel/trace/bpf_trace.c >>>> @@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr >>>> void __user *ucookies; >>>> unsigned long *addrs; >>>> u32 flags, cnt, size, cookies_size; >>>> - void __user *uaddrs; >>>> + u64 __user *uaddrs; >>>> u64 *cookies = NULL; >>>> void __user *usyms; >>>> int err; >>>> @@ -2486,9 +2486,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr >>>> return -ENOMEM; >>>> >>>> if (uaddrs) { >>>> - if (copy_from_user(addrs, uaddrs, size)) { >>>> - err = -EFAULT; >>>> - goto error; >>>> + if (sizeof(*addrs) == sizeof(*uaddrs)) { >>>> + if (copy_from_user(addrs, uaddrs, size)) { >>>> + err = -EFAULT; >>>> + goto error; >>>> + } >>>> + } else { >>>> + u32 i; >>>> + u64 addr; >>>> + >>>> + for (i = 0; i < cnt; i++) { >>>> + if (get_user(addr, uaddrs + i)) { >>>> + err = -EFAULT; >>>> + goto error; >>>> + } >>>> + if (addr > ULONG_MAX) { >>>> + err = -EINVAL; >>>> + goto error; >>>> + } >>>> + addrs[i] = addr; >>>> + } >>>> } >>>> } else { >>>> struct user_syms us; >>>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h >>>> index 2e0d373..da9c6037 100644 >>>> --- a/tools/lib/bpf/bpf.h >>>> +++ b/tools/lib/bpf/bpf.h >>>> @@ -418,7 +418,7 @@ struct bpf_link_create_opts { >>>> __u32 flags; >>>> __u32 cnt; >>>> const char **syms; >>>> - const unsigned long *addrs; >>>> + const __u64 *addrs; >>>> const __u64 *cookies; >>>> } kprobe_multi; >>>> struct { >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >>>> index ef7f302..35fa9c5 100644 >>>> --- a/tools/lib/bpf/libbpf.c >>>> +++ b/tools/lib/bpf/libbpf.c >>>> @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat) >>>> >>>> struct kprobe_multi_resolve { >>>> const char *pattern; >>>> - unsigned long *addrs; >>>> + __u64 *addrs; >>>> size_t cap; >>>> size_t cnt; >>>> }; >>>> @@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type, >>>> if (!glob_match(sym_name, res->pattern)) >>>> return 0; >>>> >>>> - err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long), >>>> + err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64), >>>> res->cnt + 1); >>>> if (err) >>>> return err; >>>> >>>> - res->addrs[res->cnt++] = (unsigned long) sym_addr; >>>> + res->addrs[res->cnt++] = sym_addr; >>>> return 0; >>>> } [...]
| |