[PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[Eugene Syromiatnikov]

Since bpf_kprobe_multi_link_attach doesn't support 32-bit kernels
for whatever reason, having it enabled for compat processes on 64-bit
kernels makes even less sense due to discrepances in the type sizes
that it does not handle.

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

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d8553f4..9560af6 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2410,7 +2410,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 	int err;
 
 	/* no support for 32bit archs yet */
-	if (sizeof(u64) != sizeof(void *))
+	if (sizeof(u64) != sizeof(void *) || in_compat_syscall())
 		return -EOPNOTSUPP;
 
 	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
-- 
2.1.4

Re: [PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[Alexei Starovoitov]

On Fri, May 6, 2022 at 7:22 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> Since bpf_kprobe_multi_link_attach doesn't support 32-bit kernels
> for whatever reason,

Jiri,
why did you add this restriction?

> having it enabled for compat processes on 64-bit
> kernels makes even less sense due to discrepances in the type sizes
> that it does not handle.

I don't follow this logic.
bpf progs are always 64-bit. Even when user space is 32-bit.
Jiri's check is for the kernel.

> Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>  kernel/trace/bpf_trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d8553f4..9560af6 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2410,7 +2410,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>         int err;
>
>         /* no support for 32bit archs yet */
> -       if (sizeof(u64) != sizeof(void *))
> +       if (sizeof(u64) != sizeof(void *) || in_compat_syscall())
>                 return -EOPNOTSUPP;
>
>         if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
> --
> 2.1.4
>

Re: [PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[Eugene Syromiatnikov]

On Tue, May 10, 2022 at 11:10:35AM -0700, Alexei Starovoitov wrote:
> On Fri, May 6, 2022 at 7:22 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> >
> > Since bpf_kprobe_multi_link_attach doesn't support 32-bit kernels
> > for whatever reason,
> 
> Jiri,
> why did you add this restriction?
> 
> > having it enabled for compat processes on 64-bit
> > kernels makes even less sense due to discrepances in the type sizes
> > that it does not handle.
> 
> I don't follow this logic.
> bpf progs are always 64-bit. Even when user space is 32-bit.
> Jiri's check is for the kernel.

The interface as defined (and implemented in libbpf) expects arrays of userspace
pointers to be passed (for example, syms points to an array of userspace
pointers—character strings; same goes for addrs, but with generic userspace
pointers) without regard to possible difference in the pointer size in case
of compat userspace.

> > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
> > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> > ---
> >  kernel/trace/bpf_trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index d8553f4..9560af6 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2410,7 +2410,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >         int err;
> >
> >         /* no support for 32bit archs yet */
> > -       if (sizeof(u64) != sizeof(void *))
> > +       if (sizeof(u64) != sizeof(void *) || in_compat_syscall())
> >                 return -EOPNOTSUPP;
> >
> >         if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
> > --
> > 2.1.4
> >
> 

Re: [PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[Alexei Starovoitov]

On Tue, May 10, 2022 at 11:42 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> On Tue, May 10, 2022 at 11:10:35AM -0700, Alexei Starovoitov wrote:
> > On Fri, May 6, 2022 at 7:22 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > >
> > > Since bpf_kprobe_multi_link_attach doesn't support 32-bit kernels
> > > for whatever reason,
> >
> > Jiri,
> > why did you add this restriction?
> >
> > > having it enabled for compat processes on 64-bit
> > > kernels makes even less sense due to discrepances in the type sizes
> > > that it does not handle.
> >
> > I don't follow this logic.
> > bpf progs are always 64-bit. Even when user space is 32-bit.
> > Jiri's check is for the kernel.
>
> The interface as defined (and implemented in libbpf) expects arrays of userspace
> pointers to be passed (for example, syms points to an array of userspace
> pointers—character strings; same goes for addrs, but with generic userspace
> pointers) without regard to possible difference in the pointer size in case
> of compat userspace.

I see. So kprobe_multi.syms and kprobe_multi.addrs will be 'long'
and 32-bit user space will have an issue with the 64-bit kernel.
Let's fix it properly.
We can remove sizeof(u64) != sizeof(void *) and keep libbpf as-is
by keeping syms and addrs 'long' in uapi.
As far as I can see 32-bit user space on a 32-bit kernel
should work with existing code.
in_compat_syscall() can be used to extend addrs/syms.

Re: [PATCH bpf] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat

[Jiri Olsa]

On Tue, May 10, 2022 at 03:30:10PM -0700, Alexei Starovoitov wrote:
> On Tue, May 10, 2022 at 11:42 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> >
> > On Tue, May 10, 2022 at 11:10:35AM -0700, Alexei Starovoitov wrote:
> > > On Fri, May 6, 2022 at 7:22 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > > >
> > > > Since bpf_kprobe_multi_link_attach doesn't support 32-bit kernels
> > > > for whatever reason,
> > >
> > > Jiri,
> > > why did you add this restriction?

sorry, I overlooked this email

the reason for that check is that we link addrs array with cookies
which are u64 and we need to swap cookies together with addrs when
we sort them

but now when I look at that, that could perhaps work event if
unsigned long is 32 bits, will check

> > >
> > > > having it enabled for compat processes on 64-bit
> > > > kernels makes even less sense due to discrepances in the type sizes
> > > > that it does not handle.
> > >
> > > I don't follow this logic.
> > > bpf progs are always 64-bit. Even when user space is 32-bit.
> > > Jiri's check is for the kernel.
> >
> > The interface as defined (and implemented in libbpf) expects arrays of userspace
> > pointers to be passed (for example, syms points to an array of userspace
> > pointers—character strings; same goes for addrs, but with generic userspace
> > pointers) without regard to possible difference in the pointer size in case
> > of compat userspace.
> 
> I see. So kprobe_multi.syms and kprobe_multi.addrs will be 'long'
> and 32-bit user space will have an issue with the 64-bit kernel.
> Let's fix it properly.
> We can remove sizeof(u64) != sizeof(void *) and keep libbpf as-is
> by keeping syms and addrs 'long' in uapi.
> As far as I can see 32-bit user space on a 32-bit kernel
> should work with existing code.
> in_compat_syscall() can be used to extend addrs/syms.

I'll check Eugene's new patchset

jirka