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@xxxxxxxxxx>
> > > ---
> > > 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
> > >
> >
>
