From: Jiri Olsa <olsajiri@gmail.com>
To: Eugene Syromiatnikov <esyr@redhat.com>
Cc: Jiri Olsa <olsajiri@gmail.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
netdev@vger.kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org, Shuah Khan <shuah@kernel.org>,
linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
Date: Tue, 17 May 2022 22:03:04 +0200 [thread overview]
Message-ID: <YoP/eEMqAn3sVFXf@krava> (raw)
In-Reply-To: <20220517123050.GA25149@asgard.redhat.com>
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
> > >
> >
>
next prev parent reply other threads:[~2022-05-17 20:03 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-17 7:36 [PATCH bpf-next v3 0/4] Fix 32-bit arch and compat support for the kprobe_multi attach type Eugene Syromiatnikov
2022-05-17 7:36 ` [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach Eugene Syromiatnikov
2022-05-17 9:12 ` Jiri Olsa
2022-05-18 23:30 ` Andrii Nakryiko
2022-05-19 14:37 ` Eugene Syromiatnikov
2022-05-20 0:48 ` Andrii Nakryiko
2022-05-17 7:36 ` [PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels " Eugene Syromiatnikov
2022-05-17 9:12 ` Jiri Olsa
2022-05-18 23:31 ` Andrii Nakryiko
2022-05-17 7:36 ` [PATCH bpf-next v3 3/4] bpf_trace: handle compat in copy_user_syms Eugene Syromiatnikov
2022-05-18 23:39 ` Andrii Nakryiko
2022-05-17 7:36 ` [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs Eugene Syromiatnikov
2022-05-17 9:12 ` Jiri Olsa
2022-05-17 12:30 ` Eugene Syromiatnikov
2022-05-17 20:03 ` Jiri Olsa [this message]
2022-05-17 21:34 ` Yonghong Song
2022-05-18 11:24 ` Jiri Olsa
2022-05-18 12:30 ` Eugene Syromiatnikov
2022-05-18 23:47 ` Andrii Nakryiko
2022-05-18 23:48 ` Andrii Nakryiko
2022-05-19 17:33 ` Eugene Syromiatnikov
2022-05-20 23:16 ` Andrii Nakryiko
2022-05-18 23:50 ` Andrii Nakryiko
2022-05-19 14:43 ` Eugene Syromiatnikov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YoP/eEMqAn3sVFXf@krava \
--to=olsajiri@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=esyr@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=shuah@kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).