| Message ID | 6ef675aeeea442fa8fc168cd1cb4e4e474f65a3f.1652772731.git.esyr@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix 32-bit arch and compat support for the kprobe_multi attach type | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 11 this patch: 11 |
| netdev/cc_maintainers | success | CCed 16 of 16 maintainers |
| netdev/build_clang | success | Errors and warnings before: 9 this patch: 9 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 11 this patch: 11 |
| netdev/checkpatch | warning | CHECK: No space is necessary after a cast |
| netdev/kdoc | success | Errors and warnings before: 74 this patch: 74 |
| netdev/source_inline | success | Was 0 now: 0 |
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 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; > } > > @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > }; > struct bpf_link *link = NULL; > char errmsg[STRERR_BUFSIZE]; > - const unsigned long *addrs; > + const __u64 *addrs; > int err, link_fd, prog_fd; > const __u64 *cookies; > const char **syms; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 9e9a3fd..76e171d 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { > /* array of function symbols to attach */ > const char **syms; > /* array of function addresses to attach */ > - const unsigned long *addrs; > + const __u64 *addrs; > /* array of user-provided values fetchable through bpf_get_attach_cookie */ > const __u64 *cookies; > /* number of elements in syms/addrs/cookies arrays */ > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > index 83ef55e3..e843840 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) > cookies[6] = 7; > cookies[7] = 8; > > - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; > + opts.kprobe_multi.addrs = (const __u64 *) &addrs; > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > opts.kprobe_multi.cookies = (const __u64 *) &cookies; > prog_fd = bpf_program__fd(skel->progs.test_kprobe); > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > index 586dc52..7646112 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) > GET_ADDR("bpf_fentry_test7", addrs[6]); > GET_ADDR("bpf_fentry_test8", addrs[7]); > > - opts.kprobe_multi.addrs = (const unsigned long*) addrs; > + opts.kprobe_multi.addrs = (const __u64 *) addrs; > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > test_link_api(&opts); > } > @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) > GET_ADDR("bpf_fentry_test7", addrs[6]); > GET_ADDR("bpf_fentry_test8", addrs[7]); > > - opts.addrs = (const unsigned long *) addrs; > + opts.addrs = (const __u64 *) addrs; > opts.cnt = ARRAY_SIZE(addrs); > test_attach_api(NULL, &opts); > } > @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) > goto cleanup; > > /* fail_2 - both addrs and syms set */ > - opts.addrs = (const unsigned long *) addrs; > + opts.addrs = (const __u64 *) addrs; > opts.syms = syms; > opts.cnt = ARRAY_SIZE(syms); > opts.cookies = NULL; > @@ -258,7 +258,7 @@ static void test_attach_api_fails(void) > goto cleanup; > > /* fail_3 - pattern and addrs set */ > - opts.addrs = (const unsigned long *) addrs; > + opts.addrs = (const __u64 *) addrs; > opts.syms = NULL; > opts.cnt = ARRAY_SIZE(syms); > opts.cookies = NULL; > -- > 2.1.4 >
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). > 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; > > } > > > > @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > > }; > > struct bpf_link *link = NULL; > > char errmsg[STRERR_BUFSIZE]; > > - const unsigned long *addrs; > > + const __u64 *addrs; > > int err, link_fd, prog_fd; > > const __u64 *cookies; > > const char **syms; > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index 9e9a3fd..76e171d 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { > > /* array of function symbols to attach */ > > const char **syms; > > /* array of function addresses to attach */ > > - const unsigned long *addrs; > > + const __u64 *addrs; > > /* array of user-provided values fetchable through bpf_get_attach_cookie */ > > const __u64 *cookies; > > /* number of elements in syms/addrs/cookies arrays */ > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > index 83ef55e3..e843840 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) > > cookies[6] = 7; > > cookies[7] = 8; > > > > - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; > > + opts.kprobe_multi.addrs = (const __u64 *) &addrs; > > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > > opts.kprobe_multi.cookies = (const __u64 *) &cookies; > > prog_fd = bpf_program__fd(skel->progs.test_kprobe); > > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > index 586dc52..7646112 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) > > GET_ADDR("bpf_fentry_test7", addrs[6]); > > GET_ADDR("bpf_fentry_test8", addrs[7]); > > > > - opts.kprobe_multi.addrs = (const unsigned long*) addrs; > > + opts.kprobe_multi.addrs = (const __u64 *) addrs; > > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > > test_link_api(&opts); > > } > > @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) > > GET_ADDR("bpf_fentry_test7", addrs[6]); > > GET_ADDR("bpf_fentry_test8", addrs[7]); > > > > - opts.addrs = (const unsigned long *) addrs; > > + opts.addrs = (const __u64 *) addrs; > > opts.cnt = ARRAY_SIZE(addrs); > > test_attach_api(NULL, &opts); > > } > > @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) > > goto cleanup; > > > > /* fail_2 - both addrs and syms set */ > > - opts.addrs = (const unsigned long *) addrs; > > + opts.addrs = (const __u64 *) addrs; > > opts.syms = syms; > > opts.cnt = ARRAY_SIZE(syms); > > opts.cookies = NULL; > > @@ -258,7 +258,7 @@ static void test_attach_api_fails(void) > > goto cleanup; > > > > /* fail_3 - pattern and addrs set */ > > - opts.addrs = (const unsigned long *) addrs; > > + opts.addrs = (const __u64 *) addrs; > > opts.syms = NULL; > > opts.cnt = ARRAY_SIZE(syms); > > opts.cookies = NULL; > > -- > > 2.1.4 > > >
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? 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; > > > } > > > > > > @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > > > }; > > > struct bpf_link *link = NULL; > > > char errmsg[STRERR_BUFSIZE]; > > > - const unsigned long *addrs; > > > + const __u64 *addrs; > > > int err, link_fd, prog_fd; > > > const __u64 *cookies; > > > const char **syms; > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > > index 9e9a3fd..76e171d 100644 > > > --- a/tools/lib/bpf/libbpf.h > > > +++ b/tools/lib/bpf/libbpf.h > > > @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { > > > /* array of function symbols to attach */ > > > const char **syms; > > > /* array of function addresses to attach */ > > > - const unsigned long *addrs; > > > + const __u64 *addrs; > > > /* array of user-provided values fetchable through bpf_get_attach_cookie */ > > > const __u64 *cookies; > > > /* number of elements in syms/addrs/cookies arrays */ > > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > > index 83ef55e3..e843840 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > > > @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) > > > cookies[6] = 7; > > > cookies[7] = 8; > > > > > > - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; > > > + opts.kprobe_multi.addrs = (const __u64 *) &addrs; > > > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > > > opts.kprobe_multi.cookies = (const __u64 *) &cookies; > > > prog_fd = bpf_program__fd(skel->progs.test_kprobe); > > > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > > index 586dc52..7646112 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > > > @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) > > > GET_ADDR("bpf_fentry_test7", addrs[6]); > > > GET_ADDR("bpf_fentry_test8", addrs[7]); > > > > > > - opts.kprobe_multi.addrs = (const unsigned long*) addrs; > > > + opts.kprobe_multi.addrs = (const __u64 *) addrs; > > > opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); > > > test_link_api(&opts); > > > } > > > @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) > > > GET_ADDR("bpf_fentry_test7", addrs[6]); > > > GET_ADDR("bpf_fentry_test8", addrs[7]); > > > > > > - opts.addrs = (const unsigned long *) addrs; > > > + opts.addrs = (const __u64 *) addrs; > > > opts.cnt = ARRAY_SIZE(addrs); > > > test_attach_api(NULL, &opts); > > > } > > > @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) > > > goto cleanup; > > > > > > /* fail_2 - both addrs and syms set */ > > > - opts.addrs = (const unsigned long *) addrs; > > > + opts.addrs = (const __u64 *) addrs; > > > opts.syms = syms; > > > opts.cnt = ARRAY_SIZE(syms); > > > opts.cookies = NULL; > > > @@ -258,7 +258,7 @@ static void test_attach_api_fails(void) > > > goto cleanup; > > > > > > /* fail_3 - pattern and addrs set */ > > > - opts.addrs = (const unsigned long *) addrs; > > > + opts.addrs = (const __u64 *) addrs; > > > opts.syms = NULL; > > > opts.cnt = ARRAY_SIZE(syms); > > > opts.cookies = NULL; > > > -- > > > 2.1.4 > > > > > >
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; >>>> } [...]
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote: > > > 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? so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees: bpf_kprobe_multi_opts::addrs as its 'unsigned long' which is 4 bytes and it needs to put there 64bits kernel addresses if we force the libbpf interface to use u64, then we should be fine > > 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? right, perhaps we could have one function to read both syms and addrs arrays > > > jirka > > > > > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes > > > > 64bit user space pointers if we have both addresses and cookies 64 then this should be ok > > > > > > > > would be gret if we could have selftest for this let's add selftest for this thanks, jirka
On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote: > On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote: > > 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? > > so the 32bit application running on 64bit kernel using libbpf won't > work at the moment, right? because it sees: > > bpf_kprobe_multi_opts::addrs as its 'unsigned long' > > which is 4 bytes and it needs to put there 64bits kernel addresses > > if we force the libbpf interface to use u64, then we should be fine Yes, that's correct. > > 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? > > right, perhaps we could have one function to read both syms and addrs arrays The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course. > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes > > > > > 64bit user space pointers > > if we have both addresses and cookies 64 then this should be ok > > > > > > > > > > > would be gret if we could have selftest for this > > let's add selftest for this Sure, I'll try to write one.
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov <esyr@redhat.com> wrote: > > On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote: > > On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote: > > > 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? > > > > so the 32bit application running on 64bit kernel using libbpf won't > > work at the moment, right? because it sees: > > > > bpf_kprobe_multi_opts::addrs as its 'unsigned long' > > > > which is 4 bytes and it needs to put there 64bits kernel addresses > > > > if we force the libbpf interface to use u64, then we should be fine > > Yes, that's correct. > > > > 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? > > > > right, perhaps we could have one function to read both syms and addrs arrays > > The distinction here it that syms are user space pointers (so they are > naturally 32-bit for 32-bit applications) and addrs are kernel-space > pointers (so they may be 64-bit even when the application is 32-bit). > Nothing prevents from changing the interface so that syms is an array > of 64-bit values treated as user space pointers, of course. I agree. User-space pointers should stay pointers in libbpf API , while kernel addresses are not really pointers for user-space app, so marking it as __u64 seems right. > > > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes > > > > > > 64bit user space pointers > > > > if we have both addresses and cookies 64 then this should be ok > > > > > > > > > > > > > > would be gret if we could have selftest for this > > > > let's add selftest for this > > Sure, I'll try to write one. >
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov <esyr@redhat.com> wrote: > > On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote: > > On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote: > > > 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? > > > > so the 32bit application running on 64bit kernel using libbpf won't > > work at the moment, right? because it sees: > > > > bpf_kprobe_multi_opts::addrs as its 'unsigned long' > > > > which is 4 bytes and it needs to put there 64bits kernel addresses > > > > if we force the libbpf interface to use u64, then we should be fine > > Yes, that's correct. > > > > 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? > > > > right, perhaps we could have one function to read both syms and addrs arrays > > The distinction here it that syms are user space pointers (so they are > naturally 32-bit for 32-bit applications) and addrs are kernel-space > pointers (so they may be 64-bit even when the application is 32-bit). > Nothing prevents from changing the interface so that syms is an array > of 64-bit values treated as user space pointers, of course. > > > > > > > we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes > > > > > > 64bit user space pointers > > > > if we have both addresses and cookies 64 then this should be ok > > > > > > > > > > > > > > would be gret if we could have selftest for this > > > > let's add selftest for this > > Sure, I'll try to write one. > Not sure how you can do that without having extra test_progs variant that's running in compat mode?
On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov <esyr@redhat.com> 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(). > > 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 ++++++++++++++++++---- kernel changes should go into a separate patch (and seems like they logically fit together with patch #3, no?) > 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(-) > [...]
On Wed, May 18, 2022 at 04:50:58PM -0700, Andrii Nakryiko wrote: > On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov <esyr@redhat.com> 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(). > > > > 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 ++++++++++++++++++---- > > kernel changes should go into a separate patch Sure, they can be split, the only reason they are this way is to keep API/ABI in sync between the kernel code and the user space one. > (and seems like they > logically fit together with patch #3, no?) Patch #3 doesn't change the API/ABI, it only fixes the implementation in terms of compat handling (and it is more straightforward), that is why I decided to have it separately. The compat handling of addrs, on the other hand, can't be fixed without the ABI change. > > 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(-) > > > > [...] >
On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote: > Not sure how you can do that without having extra test_progs variant > that's running in compat mode? I think, all bpf selftests are to be run in compat mode as well, now is a good time to enable this as any.
On Thu, May 19, 2022 at 10:34 AM Eugene Syromiatnikov <esyr@redhat.com> wrote: > > On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote: > > Not sure how you can do that without having extra test_progs variant > > that's running in compat mode? > > I think, all bpf selftests are to be run in compat mode as well, > now is a good time to enable this as any. > It's going to add a noticeable delay to CI runs, which is bad. Until we have everything set up to run test_progs flavors in parallel, adding compat flavor is not an option.
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; } @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, }; struct bpf_link *link = NULL; char errmsg[STRERR_BUFSIZE]; - const unsigned long *addrs; + const __u64 *addrs; int err, link_fd, prog_fd; const __u64 *cookies; const char **syms; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 9e9a3fd..76e171d 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { /* array of function symbols to attach */ const char **syms; /* array of function addresses to attach */ - const unsigned long *addrs; + const __u64 *addrs; /* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies; /* number of elements in syms/addrs/cookies arrays */ diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 83ef55e3..e843840 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6] = 7; cookies[7] = 8; - opts.kprobe_multi.addrs = (const unsigned long *) &addrs; + opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd = bpf_program__fd(skel->progs.test_kprobe); diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index 586dc52..7646112 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]); - opts.kprobe_multi.addrs = (const unsigned long*) addrs; + opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); test_link_api(&opts); } @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]); - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.cnt = ARRAY_SIZE(addrs); test_attach_api(NULL, &opts); } @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_2 - both addrs and syms set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL; @@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_3 - pattern and addrs set */ - opts.addrs = (const unsigned long *) addrs; + opts.addrs = (const __u64 *) addrs; opts.syms = NULL; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
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(). 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(-)