[PATCH bpf-next v3 0/4] Fix 32-bit arch and compat support for the kprobe_multi attach type

[PATCH bpf-next v3 0/4] Fix 32-bit arch and compat support for the kprobe_multi attach type

[Eugene Syromiatnikov]

As suggested in [1], the kprobe_multi interface is to be fixed for 32-bit
architectures and compat, rather then disabled.  As it turned out,
there are a couple of additional problems that are to be addressed:
 - the absence of size overflow checks, leading to possible
   out-of-bounds writes (addressed by the first patch; this one likely has
   to be fixed in 5.18, where the version of the patch from [3]
   may be preferrable, along with [4] to avoid applying the rest
   of the series);
 - the assumption that long has the same size as u64, which would make
   cookies arrays size calculation incorrect on 32-bit architectures
   (addressed by the second patch);
 - the addrs array passing API, that is incompatible with compat and has
   to be changed (addressed in the fourth patch): those are kernel
   addresses and not user ones (as was incorrectly stated in [2]);
   this change is only semantical for 64-bit user/kernelspace,
   so it shouldn't impact ABI there, at least.

[1] https://lore.kernel.org/lkml/CAADnVQ+2gwhcMht4PuDnDOFKY68Wsq8QFz4Y69NBX_TLaSexQQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/20220510184155.GA8295@asgard.redhat.com/
[3] https://lore.kernel.org/lkml/20220516230455.GA25103@asgard.redhat.com/
[4] https://lore.kernel.org/lkml/20220506142148.GA24802@asgard.redhat.com/

v3:
 - Rebased on top of bpf-next
 - Removed unnecessary size/cookies_size assignments as suggested
   by Yonghong Sond

v2: https://lore.kernel.org/lkml/20220516230441.GA22091@asgard.redhat.com/
 - Fixed the isses reported by CI

v1: https://lore.kernel.org/lkml/20220516182657.GA28596@asgard.redhat.com/

Eugene Syromiatnikov (4):
  bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
  bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
  bpf_trace: handle compat in copy_user_syms
  bpf_trace: pass array of u64 values in kprobe_multi.addrs

 kernel/trace/bpf_trace.c                           | 67 ++++++++++++++++------
 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, 62 insertions(+), 27 deletions(-)

-- 
2.1.4

[PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach

[Eugene Syromiatnikov]

Check that size would not overflow before calculation (and return
-EOVERFLOW if it will), to prevent potential out-of-bounds write
with the following copy_from_user.  Use kvmalloc_array
in copy_user_syms to prevent out-of-bounds write into syms
(and especially buf) as well.

Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Cc: <stable@vger.kernel.org> # 5.18
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
 kernel/trace/bpf_trace.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 7141ca8..9c041be 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
 	int err = -ENOMEM;
 	unsigned int i;
 
-	syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
+	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
 	if (!syms)
 		goto error;
 
-	buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
+	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
 	if (!buf)
 		goto error;
 
@@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (!cnt)
 		return -EINVAL;
 
-	size = cnt * sizeof(*addrs);
+	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
+		return -EOVERFLOW;
 	addrs = kvmalloc(size, GFP_KERNEL);
 	if (!addrs)
 		return -ENOMEM;
-- 
2.1.4

Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach

[Jiri Olsa]

On Tue, May 17, 2022 at 09:36:15AM +0200, Eugene Syromiatnikov wrote:
> Check that size would not overflow before calculation (and return
> -EOVERFLOW if it will), to prevent potential out-of-bounds write
> with the following copy_from_user.  Use kvmalloc_array
> in copy_user_syms to prevent out-of-bounds write into syms
> (and especially buf) as well.
> 
> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> Cc: <stable@vger.kernel.org> # 5.18
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  kernel/trace/bpf_trace.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 7141ca8..9c041be 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>  	int err = -ENOMEM;
>  	unsigned int i;
>  
> -	syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> +	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
>  	if (!syms)
>  		goto error;
>  
> -	buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> +	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
>  	if (!buf)
>  		goto error;
>  
> @@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	if (!cnt)
>  		return -EINVAL;
>  
> -	size = cnt * sizeof(*addrs);
> +	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> +		return -EOVERFLOW;
>  	addrs = kvmalloc(size, GFP_KERNEL);
>  	if (!addrs)
>  		return -ENOMEM;
> -- 
> 2.1.4
> 

Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach

[Andrii Nakryiko]

On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> Check that size would not overflow before calculation (and return
> -EOVERFLOW if it will), to prevent potential out-of-bounds write
> with the following copy_from_user.  Use kvmalloc_array
> in copy_user_syms to prevent out-of-bounds write into syms
> (and especially buf) as well.
>
> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> Cc: <stable@vger.kernel.org> # 5.18
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>  kernel/trace/bpf_trace.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 7141ca8..9c041be 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>         int err = -ENOMEM;
>         unsigned int i;
>
> -       syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> +       syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
>         if (!syms)
>                 goto error;
>
> -       buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> +       buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
>         if (!buf)
>                 goto error;
>
> @@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>         if (!cnt)
>                 return -EINVAL;
>
> -       size = cnt * sizeof(*addrs);
> +       if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> +               return -EOVERFLOW;
>         addrs = kvmalloc(size, GFP_KERNEL);

any good reason not to use kvmalloc_array() here as well and delegate
overflow to it. And then use long size (as expected by copy_from_user
anyway) everywhere?

>         if (!addrs)
>                 return -ENOMEM;
> --
> 2.1.4
>

Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach

[Eugene Syromiatnikov]

On Wed, May 18, 2022 at 04:30:14PM -0700, Andrii Nakryiko wrote:
> On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> >
> > Check that size would not overflow before calculation (and return
> > -EOVERFLOW if it will), to prevent potential out-of-bounds write
> > with the following copy_from_user.  Use kvmalloc_array
> > in copy_user_syms to prevent out-of-bounds write into syms
> > (and especially buf) as well.
> >
> > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> > Cc: <stable@vger.kernel.org> # 5.18
> > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> > ---
> >  kernel/trace/bpf_trace.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 7141ca8..9c041be 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> >         int err = -ENOMEM;
> >         unsigned int i;
> >
> > -       syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> > +       syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
> >         if (!syms)
> >                 goto error;
> >
> > -       buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> > +       buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
> >         if (!buf)
> >                 goto error;
> >
> > @@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >         if (!cnt)
> >                 return -EINVAL;
> >
> > -       size = cnt * sizeof(*addrs);
> > +       if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> > +               return -EOVERFLOW;
> >         addrs = kvmalloc(size, GFP_KERNEL);
> 
> any good reason not to use kvmalloc_array() here as well and delegate
> overflow to it. And then use long size (as expected by copy_from_user
> anyway) everywhere?

Just to avoid double calculation of size, otherwise I don't have
any significant prefernce, other than -EOVERFLOW would not be reported
separately (not sure if this a good or a bad thing), and that
it would be a bit more cumbersome to incorporate the Yonghong's
suggestion[1] about the INT_MAX check.

[1] https://lore.kernel.org/lkml/412bf136-6a5b-f442-1e84-778697e2b694@fb.com/

> >         if (!addrs)
> >                 return -ENOMEM;
> > --
> > 2.1.4
> >
> 

Re: [PATCH bpf-next v3 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach

[Andrii Nakryiko]

On Thu, May 19, 2022 at 7:37 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> On Wed, May 18, 2022 at 04:30:14PM -0700, Andrii Nakryiko wrote:
> > On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > >
> > > Check that size would not overflow before calculation (and return
> > > -EOVERFLOW if it will), to prevent potential out-of-bounds write
> > > with the following copy_from_user.  Use kvmalloc_array
> > > in copy_user_syms to prevent out-of-bounds write into syms
> > > (and especially buf) as well.
> > >
> > > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> > > Cc: <stable@vger.kernel.org> # 5.18
> > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> > > ---
> > >  kernel/trace/bpf_trace.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 7141ca8..9c041be 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2261,11 +2261,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
> > >         int err = -ENOMEM;
> > >         unsigned int i;
> > >
> > > -       syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> > > +       syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
> > >         if (!syms)
> > >                 goto error;
> > >
> > > -       buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> > > +       buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
> > >         if (!buf)
> > >                 goto error;
> > >
> > > @@ -2461,7 +2461,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > >         if (!cnt)
> > >                 return -EINVAL;
> > >
> > > -       size = cnt * sizeof(*addrs);
> > > +       if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> > > +               return -EOVERFLOW;
> > >         addrs = kvmalloc(size, GFP_KERNEL);
> >
> > any good reason not to use kvmalloc_array() here as well and delegate
> > overflow to it. And then use long size (as expected by copy_from_user
> > anyway) everywhere?
>
> Just to avoid double calculation of size, otherwise I don't have
> any significant prefernce, other than -EOVERFLOW would not be reported
> separately (not sure if this a good or a bad thing), and that
> it would be a bit more cumbersome to incorporate the Yonghong's
> suggestion[1] about the INT_MAX check.
>

I think it's totally fine to return ENOMEM if someone requested some
unreasonable amount of symbols. And INT_MAX won't be necessary if we
delegate all the overflow checking to kvmalloc_array()

> [1] https://lore.kernel.org/lkml/412bf136-6a5b-f442-1e84-778697e2b694@fb.com/
>
> > >         if (!addrs)
> > >                 return -ENOMEM;
> > > --
> > > 2.1.4
> > >
> >
>

[PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach

[Eugene Syromiatnikov]

It seems that there is no reason not to support 32-bit architectures;
doing so requires a bit of rework with respect to cookies handling,
however, as the current code implicitly assumes
that sizeof(long) == sizeof(u64).

Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
 kernel/trace/bpf_trace.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9c041be..a93a54f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	struct bpf_link_primer link_primer;
 	void __user *ucookies;
 	unsigned long *addrs;
-	u32 flags, cnt, size;
+	u32 flags, cnt, size, cookies_size;
 	void __user *uaddrs;
 	u64 *cookies = NULL;
 	void __user *usyms;
 	int err;
 
-	/* no support for 32bit archs yet */
-	if (sizeof(u64) != sizeof(void *))
-		return -EOPNOTSUPP;
-
 	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
 		return -EINVAL;
 
@@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 
 	uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
 	usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
+	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
 	if (!!uaddrs == !!usyms)
 		return -EINVAL;
 
@@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	if (!cnt)
 		return -EINVAL;
 
-	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
+	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) ||
+	    (ucookies &&
+	     check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) {
 		return -EOVERFLOW;
+	}
 	addrs = kvmalloc(size, GFP_KERNEL);
 	if (!addrs)
 		return -ENOMEM;
@@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 			goto error;
 	}
 
-	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
 	if (ucookies) {
-		cookies = kvmalloc(size, GFP_KERNEL);
+		cookies = kvmalloc(cookies_size, GFP_KERNEL);
 		if (!cookies) {
 			err = -ENOMEM;
 			goto error;
 		}
-		if (copy_from_user(cookies, ucookies, size)) {
+		if (copy_from_user(cookies, ucookies, cookies_size)) {
 			err = -EFAULT;
 			goto error;
 		}
-- 
2.1.4

Re: [PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach

[Jiri Olsa]

On Tue, May 17, 2022 at 09:36:26AM +0200, Eugene Syromiatnikov wrote:
> It seems that there is no reason not to support 32-bit architectures;
> doing so requires a bit of rework with respect to cookies handling,
> however, as the current code implicitly assumes
> that sizeof(long) == sizeof(u64).
> 
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>  kernel/trace/bpf_trace.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9c041be..a93a54f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	struct bpf_link_primer link_primer;
>  	void __user *ucookies;
>  	unsigned long *addrs;
> -	u32 flags, cnt, size;
> +	u32 flags, cnt, size, cookies_size;
>  	void __user *uaddrs;
>  	u64 *cookies = NULL;
>  	void __user *usyms;
>  	int err;
>  
> -	/* no support for 32bit archs yet */
> -	if (sizeof(u64) != sizeof(void *))
> -		return -EOPNOTSUPP;
> -
>  	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
>  		return -EINVAL;
>  
> @@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  
>  	uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
>  	usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
> +	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>  	if (!!uaddrs == !!usyms)
>  		return -EINVAL;
>  
> @@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  	if (!cnt)
>  		return -EINVAL;
>  
> -	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> +	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) ||
> +	    (ucookies &&
> +	     check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) {
>  		return -EOVERFLOW;
> +	}
>  	addrs = kvmalloc(size, GFP_KERNEL);
>  	if (!addrs)
>  		return -ENOMEM;
> @@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  			goto error;
>  	}
>  
> -	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>  	if (ucookies) {

could we check all that in here? so the ucookies checks are on the
one place.. also you would not need cookies_size

jirka

> -		cookies = kvmalloc(size, GFP_KERNEL);
> +		cookies = kvmalloc(cookies_size, GFP_KERNEL);
>  		if (!cookies) {
>  			err = -ENOMEM;
>  			goto error;
>  		}
> -		if (copy_from_user(cookies, ucookies, size)) {
> +		if (copy_from_user(cookies, ucookies, cookies_size)) {
>  			err = -EFAULT;
>  			goto error;
>  		}
> -- 
> 2.1.4
> 

Re: [PATCH bpf-next v3 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach

[Andrii Nakryiko]

On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> It seems that there is no reason not to support 32-bit architectures;
> doing so requires a bit of rework with respect to cookies handling,
> however, as the current code implicitly assumes
> that sizeof(long) == sizeof(u64).
>
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>  kernel/trace/bpf_trace.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9c041be..a93a54f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>         struct bpf_link_primer link_primer;
>         void __user *ucookies;
>         unsigned long *addrs;
> -       u32 flags, cnt, size;
> +       u32 flags, cnt, size, cookies_size;
>         void __user *uaddrs;
>         u64 *cookies = NULL;
>         void __user *usyms;
>         int err;
>
> -       /* no support for 32bit archs yet */
> -       if (sizeof(u64) != sizeof(void *))
> -               return -EOPNOTSUPP;
> -
>         if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
>                 return -EINVAL;
>
> @@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>
>         uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
>         usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
> +       ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>         if (!!uaddrs == !!usyms)
>                 return -EINVAL;
>
> @@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>         if (!cnt)
>                 return -EINVAL;
>
> -       if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> +       if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) ||
> +           (ucookies &&
> +            check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) {
>                 return -EOVERFLOW;
> +       }
>         addrs = kvmalloc(size, GFP_KERNEL);
>         if (!addrs)
>                 return -ENOMEM;
> @@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                         goto error;
>         }
>
> -       ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>         if (ucookies) {
> -               cookies = kvmalloc(size, GFP_KERNEL);
> +               cookies = kvmalloc(cookies_size, GFP_KERNEL);

same question about consistent use of kvmalloc_array() and delegating
all the overflow checks to it?

>                 if (!cookies) {
>                         err = -ENOMEM;
>                         goto error;
>                 }
> -               if (copy_from_user(cookies, ucookies, size)) {
> +               if (copy_from_user(cookies, ucookies, cookies_size)) {
>                         err = -EFAULT;
>                         goto error;
>                 }
> --
> 2.1.4
>

[PATCH bpf-next v3 3/4] bpf_trace: handle compat in copy_user_syms

[Eugene Syromiatnikov]

For compat processes, userspace size for syms pointers is different.
Provide compat handling for copying array elements from the user space.

Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
 kernel/trace/bpf_trace.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a93a54f..9d3028a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2253,6 +2253,24 @@ struct user_syms {
 	char *buf;
 };
 
+static inline int get_arr_ptr(unsigned long *p,
+			      unsigned long __user *uaddr, u32 idx)
+{
+	if (unlikely(in_compat_syscall())) {
+		compat_uptr_t __user *compat_uaddr = (compat_uptr_t __user *)uaddr;
+		compat_uptr_t val;
+		int err;
+
+		err = __get_user(val, compat_uaddr + idx);
+		if (!err)
+			*p = val;
+
+		return err;
+	} else {
+		return __get_user(*p, uaddr + idx);
+	}
+}
+
 static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
 {
 	unsigned long __user usymbol;
@@ -2270,7 +2288,7 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
 		goto error;
 
 	for (p = buf, i = 0; i < cnt; i++) {
-		if (__get_user(usymbol, usyms + i)) {
+		if (get_arr_ptr(&usymbol, usyms, i)) {
 			err = -EFAULT;
 			goto error;
 		}
-- 
2.1.4

Re: [PATCH bpf-next v3 3/4] bpf_trace: handle compat in copy_user_syms

[Andrii Nakryiko]

On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> For compat processes, userspace size for syms pointers is different.
> Provide compat handling for copying array elements from the user space.
>
> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>  kernel/trace/bpf_trace.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a93a54f..9d3028a 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2253,6 +2253,24 @@ struct user_syms {
>         char *buf;
>  };
>
> +static inline int get_arr_ptr(unsigned long *p,
> +                             unsigned long __user *uaddr, u32 idx)

no need for inline, let compiler decide on inlining

> +{
> +       if (unlikely(in_compat_syscall())) {

not sure unlikely() is justified for code...

> +               compat_uptr_t __user *compat_uaddr = (compat_uptr_t __user *)uaddr;
> +               compat_uptr_t val;
> +               int err;
> +
> +               err = __get_user(val, compat_uaddr + idx);
> +               if (!err)
> +                       *p = val;
> +
> +               return err;
> +       } else {
> +               return __get_user(*p, uaddr + idx);
> +       }
> +}
> +
>  static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
>  {
>         unsigned long __user usymbol;
> @@ -2270,7 +2288,7 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>                 goto error;
>
>         for (p = buf, i = 0; i < cnt; i++) {
> -               if (__get_user(usymbol, usyms + i)) {
> +               if (get_arr_ptr(&usymbol, usyms, i)) {
>                         err = -EFAULT;
>                         goto error;
>                 }
> --
> 2.1.4
>

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

[Eugene Syromiatnikov]

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(-)

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

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

[Jiri Olsa]

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
> 

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

[Eugene Syromiatnikov]

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

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

[Jiri Olsa]

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

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

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

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

[Jiri Olsa]

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

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

[Eugene Syromiatnikov]

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.

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

[Andrii Nakryiko]

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.
>

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

[Andrii Nakryiko]

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?

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

[Eugene Syromiatnikov]

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.

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

[Andrii Nakryiko]

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.

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

[Andrii Nakryiko]

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(-)
>

[...]

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

[Eugene Syromiatnikov]

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(-)
> >
> 
> [...]
>