Re: [PATCH bpf-next v3 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs

[Yonghong Song <yhs@xxxxxx>]

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@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;
> > > >   }
[...]