[PATCH bpf-next v3 0/4] Fix 32-bit arch and compat support for the kprobe_multi attach type
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_TLaS... [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(-)
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;
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;-- 2.1.4
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
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
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
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; }
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))) {- } 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 (copy_from_user(cookies, ucookies, size)) {
if (copy_from_user(cookies, ucookies, cookies_size)) { err = -EFAULT; goto error;-- 2.1.4
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
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; }
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
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;
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;}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;diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ef7f302..35fa9c5 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat) struct kprobe_multi_resolve { const char *pattern;
- unsigned long *addrs;
- __u64 *addrs; size_t cap; size_t cnt;
}; @@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type, if (!glob_match(sym_name, res->pattern)) return 0;
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64), res->cnt + 1); if (err) return err;
- res->addrs[res->cnt++] = (unsigned long) sym_addr;
- res->addrs[res->cnt++] = sym_addr; return 0;
} @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, }; struct bpf_link *link = NULL; char errmsg[STRERR_BUFSIZE];
- const unsigned long *addrs;
- const __u64 *addrs; int err, link_fd, prog_fd; const __u64 *cookies; const char **syms;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 9e9a3fd..76e171d 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { /* array of function symbols to attach */ const char **syms; /* array of function addresses to attach */
- const unsigned long *addrs;
- const __u64 *addrs; /* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies; /* number of elements in syms/addrs/cookies arrays */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 83ef55e3..e843840 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6] = 7; cookies[7] = 8;
- opts.kprobe_multi.addrs = (const unsigned long *) &addrs;
- opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd = bpf_program__fd(skel->progs.test_kprobe);
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index 586dc52..7646112 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.kprobe_multi.addrs = (const unsigned long*) addrs;
- opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); test_link_api(&opts);
} @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.cnt = ARRAY_SIZE(addrs); test_attach_api(NULL, &opts);
} @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_2 - both addrs and syms set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_3 - pattern and addrs set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = NULL; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
-- 2.1.4
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
would be gret if we could have selftest for this
thanks, jirka
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") Signed-off-by: Eugene Syromiatnikov esyr@redhat.com
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++---- tools/lib/bpf/bpf.h | 2 +- tools/lib/bpf/libbpf.c | 8 +++---- tools/lib/bpf/libbpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- 6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 9d3028a..30a15b3 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr void __user *ucookies; unsigned long *addrs; u32 flags, cnt, size, cookies_size;
- void __user *uaddrs;
- u64 __user *uaddrs; u64 *cookies = NULL; void __user *usyms; int err;
@@ -2486,9 +2486,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr return -ENOMEM; if (uaddrs) {
if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;
if (sizeof(*addrs) == sizeof(*uaddrs)) {if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;}} else {u32 i;u64 addr;for (i = 0; i < cnt; i++) {if (get_user(addr, uaddrs + i)) {err = -EFAULT;goto error;}if (addr > ULONG_MAX) {err = -EINVAL;goto error;}addrs[i] = addr;}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;diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ef7f302..35fa9c5 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat) struct kprobe_multi_resolve { const char *pattern;
- unsigned long *addrs;
- __u64 *addrs; size_t cap; size_t cnt;
}; @@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type, if (!glob_match(sym_name, res->pattern)) return 0;
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64), res->cnt + 1); if (err) return err;
- res->addrs[res->cnt++] = (unsigned long) sym_addr;
- res->addrs[res->cnt++] = sym_addr; return 0;
} @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, }; struct bpf_link *link = NULL; char errmsg[STRERR_BUFSIZE];
- const unsigned long *addrs;
- const __u64 *addrs; int err, link_fd, prog_fd; const __u64 *cookies; const char **syms;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 9e9a3fd..76e171d 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { /* array of function symbols to attach */ const char **syms; /* array of function addresses to attach */
- const unsigned long *addrs;
- const __u64 *addrs; /* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies; /* number of elements in syms/addrs/cookies arrays */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 83ef55e3..e843840 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6] = 7; cookies[7] = 8;
- opts.kprobe_multi.addrs = (const unsigned long *) &addrs;
- opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd = bpf_program__fd(skel->progs.test_kprobe);
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index 586dc52..7646112 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.kprobe_multi.addrs = (const unsigned long*) addrs;
- opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); test_link_api(&opts);
} @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.cnt = ARRAY_SIZE(addrs); test_attach_api(NULL, &opts);
} @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_2 - both addrs and syms set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_3 - pattern and addrs set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = NULL; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
-- 2.1.4
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
jirka
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
would be gret if we could have selftest for this
thanks, jirka
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") Signed-off-by: Eugene Syromiatnikov esyr@redhat.com
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++---- tools/lib/bpf/bpf.h | 2 +- tools/lib/bpf/libbpf.c | 8 +++---- tools/lib/bpf/libbpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- 6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 9d3028a..30a15b3 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr void __user *ucookies; unsigned long *addrs; u32 flags, cnt, size, cookies_size;
- void __user *uaddrs;
- u64 __user *uaddrs; u64 *cookies = NULL; void __user *usyms; int err;
@@ -2486,9 +2486,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr return -ENOMEM; if (uaddrs) {
if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;
if (sizeof(*addrs) == sizeof(*uaddrs)) {if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;}} else {u32 i;u64 addr;for (i = 0; i < cnt; i++) {if (get_user(addr, uaddrs + i)) {err = -EFAULT;goto error;}if (addr > ULONG_MAX) {err = -EINVAL;goto error;}addrs[i] = addr;}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;diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ef7f302..35fa9c5 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat) struct kprobe_multi_resolve { const char *pattern;
- unsigned long *addrs;
- __u64 *addrs; size_t cap; size_t cnt;
}; @@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type, if (!glob_match(sym_name, res->pattern)) return 0;
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64), res->cnt + 1); if (err) return err;
- res->addrs[res->cnt++] = (unsigned long) sym_addr;
- res->addrs[res->cnt++] = sym_addr; return 0;
} @@ -10772,7 +10772,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, }; struct bpf_link *link = NULL; char errmsg[STRERR_BUFSIZE];
- const unsigned long *addrs;
- const __u64 *addrs; int err, link_fd, prog_fd; const __u64 *cookies; const char **syms;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 9e9a3fd..76e171d 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -489,7 +489,7 @@ struct bpf_kprobe_multi_opts { /* array of function symbols to attach */ const char **syms; /* array of function addresses to attach */
- const unsigned long *addrs;
- const __u64 *addrs; /* array of user-provided values fetchable through bpf_get_attach_cookie */ const __u64 *cookies; /* number of elements in syms/addrs/cookies arrays */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 83ef55e3..e843840 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -140,7 +140,7 @@ static void kprobe_multi_link_api_subtest(void) cookies[6] = 7; cookies[7] = 8;
- opts.kprobe_multi.addrs = (const unsigned long *) &addrs;
- opts.kprobe_multi.addrs = (const __u64 *) &addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); opts.kprobe_multi.cookies = (const __u64 *) &cookies; prog_fd = bpf_program__fd(skel->progs.test_kprobe);
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index 586dc52..7646112 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -108,7 +108,7 @@ static void test_link_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.kprobe_multi.addrs = (const unsigned long*) addrs;
- opts.kprobe_multi.addrs = (const __u64 *) addrs; opts.kprobe_multi.cnt = ARRAY_SIZE(addrs); test_link_api(&opts);
} @@ -186,7 +186,7 @@ static void test_attach_api_addrs(void) GET_ADDR("bpf_fentry_test7", addrs[6]); GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.cnt = ARRAY_SIZE(addrs); test_attach_api(NULL, &opts);
} @@ -244,7 +244,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_2 - both addrs and syms set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = syms; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
@@ -258,7 +258,7 @@ static void test_attach_api_fails(void) goto cleanup; /* fail_3 - pattern and addrs set */
- opts.addrs = (const unsigned long *) addrs;
- opts.addrs = (const __u64 *) addrs; opts.syms = NULL; opts.cnt = ARRAY_SIZE(syms); opts.cookies = NULL;
-- 2.1.4
On 5/17/22 1:03 PM, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
jirka
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
would be gret if we could have selftest for this
thanks, jirka
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") Signed-off-by: Eugene Syromiatnikov esyr@redhat.com
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++---- tools/lib/bpf/bpf.h | 2 +- tools/lib/bpf/libbpf.c | 8 +++---- tools/lib/bpf/libbpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- 6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 9d3028a..30a15b3 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2454,7 +2454,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr void __user *ucookies; unsigned long *addrs; u32 flags, cnt, size, cookies_size;
- void __user *uaddrs;
- u64 __user *uaddrs; u64 *cookies = NULL; void __user *usyms; int err;
@@ -2486,9 +2486,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr return -ENOMEM; if (uaddrs) {
if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;
if (sizeof(*addrs) == sizeof(*uaddrs)) {if (copy_from_user(addrs, uaddrs, size)) {err = -EFAULT;goto error;}} else {u32 i;u64 addr;for (i = 0; i < cnt; i++) {if (get_user(addr, uaddrs + i)) {err = -EFAULT;goto error;}if (addr > ULONG_MAX) {err = -EINVAL;goto error;}addrs[i] = addr;}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;diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index ef7f302..35fa9c5 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10737,7 +10737,7 @@ static bool glob_match(const char *str, const char *pat) struct kprobe_multi_resolve { const char *pattern;
- unsigned long *addrs;
- __u64 *addrs; size_t cap; size_t cnt; };
@@ -10752,12 +10752,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type, if (!glob_match(sym_name, res->pattern)) return 0;
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64), res->cnt + 1); if (err) return err;
- res->addrs[res->cnt++] = (unsigned long) sym_addr;
- res->addrs[res->cnt++] = sym_addr; return 0; }
[...]
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
On 5/17/22 1:03 PM, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrs as its 'unsigned long'
which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
right, perhaps we could have one function to read both syms and addrs arrays
jirka
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
would be gret if we could have selftest for this
let's add selftest for this
thanks, jirka
On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
On 5/17/22 1:03 PM, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrs as its 'unsigned long'
which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov esyr@redhat.com wrote:
On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
On 5/17/22 1:03 PM, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote: > With the interface as defined, it is impossible to pass 64-bit kernel > addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, > which severly limits the useability of the interface, change the ABI > to accept an array of u64 values instead of (kernel? user?) longs. > Interestingly, the rest of the libbpf infrastructure uses 64-bit values > for kallsyms addresses already, so this patch also eliminates > the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrs as its 'unsigned long'
which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
I agree. User-space pointers should stay pointers in libbpf API , while kernel addresses are not really pointers for user-space app, so marking it as __u64 seems right.
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
On Wed, May 18, 2022 at 5:30 AM Eugene Syromiatnikov esyr@redhat.com wrote:
On Wed, May 18, 2022 at 01:24:56PM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:34:55PM -0700, Yonghong Song wrote:
On 5/17/22 1:03 PM, Jiri Olsa wrote:
On Tue, May 17, 2022 at 02:30:50PM +0200, Eugene Syromiatnikov wrote:
On Tue, May 17, 2022 at 11:12:34AM +0200, Jiri Olsa wrote:
On Tue, May 17, 2022 at 09:36:47AM +0200, Eugene Syromiatnikov wrote: > With the interface as defined, it is impossible to pass 64-bit kernel > addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, > which severly limits the useability of the interface, change the ABI > to accept an array of u64 values instead of (kernel? user?) longs. > Interestingly, the rest of the libbpf infrastructure uses 64-bit values > for kallsyms addresses already, so this patch also eliminates > the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
so the problem is when we have 32bit user sace on 64bit kernel right?
I think we should keep addrs as longs in uapi and have kernel to figure out if it needs to read u32 or u64, like you did for symbols in previous patch
No, it's not possible here, as addrs are kernel addrs and not user space addrs, so user space has to explicitly pass 64-bit addresses on 64-bit kernels (or have a notion whether it is running on a 64-bit or 32-bit kernel, and form the passed array accordingly, which is against the idea of compat layer that tries to abstract it out).
hum :-\ I'll need to check on compat layer.. there must be some other code doing this already somewhere, right?
so the 32bit application running on 64bit kernel using libbpf won't work at the moment, right? because it sees:
bpf_kprobe_multi_opts::addrs as its 'unsigned long'
which is 4 bytes and it needs to put there 64bits kernel addresses
if we force the libbpf interface to use u64, then we should be fine
Yes, that's correct.
I am not familiar with all these compatibility thing. But if we have 64-bit pointer for **syms, maybe we could also have 64-bit pointer for *syms for consistency?
right, perhaps we could have one function to read both syms and addrs arrays
The distinction here it that syms are user space pointers (so they are naturally 32-bit for 32-bit applications) and addrs are kernel-space pointers (so they may be 64-bit even when the application is 32-bit). Nothing prevents from changing the interface so that syms is an array of 64-bit values treated as user space pointers, of course.
we'll need to fix also bpf_kprobe_multi_cookie_swap because it assumes 64bit user space pointers
if we have both addresses and cookies 64 then this should be ok
would be gret if we could have selftest for this
let's add selftest for this
Sure, I'll try to write one.
Not sure how you can do that without having extra test_progs variant that's running in compat mode?
On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote:
Not sure how you can do that without having extra test_progs variant that's running in compat mode?
I think, all bpf selftests are to be run in compat mode as well, now is a good time to enable this as any.
On Thu, May 19, 2022 at 10:34 AM Eugene Syromiatnikov esyr@redhat.com wrote:
On Wed, May 18, 2022 at 04:48:59PM -0700, Andrii Nakryiko wrote:
Not sure how you can do that without having extra test_progs variant that's running in compat mode?
I think, all bpf selftests are to be run in compat mode as well, now is a good time to enable this as any.
It's going to add a noticeable delay to CI runs, which is bad. Until we have everything set up to run test_progs flavors in parallel, adding compat flavor is not an option.
On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov esyr@redhat.com wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") Signed-off-by: Eugene Syromiatnikov esyr@redhat.com
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
kernel changes should go into a separate patch (and seems like they logically fit together with patch #3, no?)
tools/lib/bpf/bpf.h | 2 +- tools/lib/bpf/libbpf.c | 8 +++---- tools/lib/bpf/libbpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- 6 files changed, 32 insertions(+), 15 deletions(-)
[...]
On Wed, May 18, 2022 at 04:50:58PM -0700, Andrii Nakryiko wrote:
On Tue, May 17, 2022 at 12:37 AM Eugene Syromiatnikov esyr@redhat.com wrote:
With the interface as defined, it is impossible to pass 64-bit kernel addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI, which severly limits the useability of the interface, change the ABI to accept an array of u64 values instead of (kernel? user?) longs. Interestingly, the rest of the libbpf infrastructure uses 64-bit values for kallsyms addresses already, so this patch also eliminates the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes") Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function") Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test") Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts") Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test") Signed-off-by: Eugene Syromiatnikov esyr@redhat.com
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
kernel changes should go into a separate patch
Sure, they can be split, the only reason they are this way is to keep API/ABI in sync between the kernel code and the user space one.
(and seems like they logically fit together with patch #3, no?)
Patch #3 doesn't change the API/ABI, it only fixes the implementation in terms of compat handling (and it is more straightforward), that is why I decided to have it separately. The compat handling of addrs, on the other hand, can't be fixed without the ABI change.
tools/lib/bpf/bpf.h | 2 +- tools/lib/bpf/libbpf.c | 8 +++---- tools/lib/bpf/libbpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +- .../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++---- 6 files changed, 32 insertions(+), 15 deletions(-)
[...]
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
Eugene Syromiatnikov -
Jiri Olsa -
Yonghong Song