[RFC] bpf: Fix crash on mm_init trampoline attachment

[RFC] bpf: Fix crash on mm_init trampoline attachment

[Jiri Olsa]

There are 2 mm_init functions in kernel.

One in kernel/fork.c:
  static struct mm_struct *mm_init(struct mm_struct *mm,
                                   struct task_struct *p,
                                   struct user_namespace *user_ns)

And another one in init/main.c:
  static void __init mm_init(void)

The BTF data will get the first one, which is most likely
(in my case) mm_init from init/main.c without arguments.

Then in runtime when we want to attach to 'mm_init' the
kalsyms contains address of the one from kernel/fork.c.

So we have function model with no arguments and using it
to attach function with 3 arguments.. as result the trampoline
will not save function's arguments and we get crash because
trampoline changes argument registers:

  BUG: unable to handle page fault for address: 0000607d87a1d558
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0002) - not-present page
  PGD 0 P4D 0
  Oops: 0002 [#1] SMP PTI
  CPU: 6 PID: 936 Comm: systemd Not tainted 5.12.0-rc4qemu+ #191
  RIP: 0010:mm_init+0x223/0x2a0
  ...
  Call Trace:
   ? bpf_trampoline_6442453476_0+0x3b/0x1000
   dup_mm+0x66/0x5f0
   ? __lock_task_sighand+0x3a/0x70
   copy_process+0x17d0/0x1b50
   kernel_clone+0x97/0x3c0
   __do_sys_clone+0x60/0x80
   do_syscall_64+0x33/0x40
   entry_SYSCALL_64_after_hwframe+0x44/0xae
  RIP: 0033:0x7f1dc9b3201f

I think there might be more cases like this, but I don't have
an idea yet how to solve this in generic way. The rename in
this change fix it for this instance.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 init/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/init/main.c b/init/main.c
index 53b278845b88..bc1bfe57daf7 100644
--- a/init/main.c
+++ b/init/main.c
@@ -818,7 +818,7 @@ static void __init report_meminit(void)
 /*
  * Set up kernel memory allocators
  */
-static void __init mm_init(void)
+static void __init init_mem(void)
 {
 	/*
 	 * page_ext requires contiguous pages,
@@ -905,7 +905,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 	vfs_caches_init_early();
 	sort_main_extable();
 	trap_init();
-	mm_init();
+	init_mem();
 
 	ftrace_init();
 
-- 
2.30.2

Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

[Andrii Nakryiko]

On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> There are 2 mm_init functions in kernel.
>
> One in kernel/fork.c:
>   static struct mm_struct *mm_init(struct mm_struct *mm,
>                                    struct task_struct *p,
>                                    struct user_namespace *user_ns)
>
> And another one in init/main.c:
>   static void __init mm_init(void)
>
> The BTF data will get the first one, which is most likely
> (in my case) mm_init from init/main.c without arguments.
>
> Then in runtime when we want to attach to 'mm_init' the
> kalsyms contains address of the one from kernel/fork.c.
>
> So we have function model with no arguments and using it
> to attach function with 3 arguments.. as result the trampoline
> will not save function's arguments and we get crash because
> trampoline changes argument registers:
>
>   BUG: unable to handle page fault for address: 0000607d87a1d558
>   #PF: supervisor write access in kernel mode
>   #PF: error_code(0x0002) - not-present page
>   PGD 0 P4D 0
>   Oops: 0002 [#1] SMP PTI
>   CPU: 6 PID: 936 Comm: systemd Not tainted 5.12.0-rc4qemu+ #191
>   RIP: 0010:mm_init+0x223/0x2a0
>   ...
>   Call Trace:
>    ? bpf_trampoline_6442453476_0+0x3b/0x1000
>    dup_mm+0x66/0x5f0
>    ? __lock_task_sighand+0x3a/0x70
>    copy_process+0x17d0/0x1b50
>    kernel_clone+0x97/0x3c0
>    __do_sys_clone+0x60/0x80
>    do_syscall_64+0x33/0x40
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
>   RIP: 0033:0x7f1dc9b3201f
>
> I think there might be more cases like this, but I don't have
> an idea yet how to solve this in generic way. The rename in
> this change fix it for this instance.

Just retroactively renaming functions and waiting for someone else to
report similar issues is probably not the best strategy. Should
resolve_btfids detect all name duplicates and emit warnings for them?
It would be good to also remove such name-conflicting FUNCs from BTF
(though currently it's not easy). And fail if such a function is
referenced from .BTF_ids section.

Thoughts?

>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  init/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 53b278845b88..bc1bfe57daf7 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -818,7 +818,7 @@ static void __init report_meminit(void)
>  /*
>   * Set up kernel memory allocators
>   */
> -static void __init mm_init(void)
> +static void __init init_mem(void)
>  {
>         /*
>          * page_ext requires contiguous pages,
> @@ -905,7 +905,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>         vfs_caches_init_early();
>         sort_main_extable();
>         trap_init();
> -       mm_init();
> +       init_mem();

nit: given trap_init and ftrace_init, mem_init probably would be a better name?

>
>         ftrace_init();
>
> --
> 2.30.2
>

Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

[Jiri Olsa]

On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:
> On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > There are 2 mm_init functions in kernel.
> >
> > One in kernel/fork.c:
> >   static struct mm_struct *mm_init(struct mm_struct *mm,
> >                                    struct task_struct *p,
> >                                    struct user_namespace *user_ns)
> >
> > And another one in init/main.c:
> >   static void __init mm_init(void)
> >
> > The BTF data will get the first one, which is most likely
> > (in my case) mm_init from init/main.c without arguments.
> >
> > Then in runtime when we want to attach to 'mm_init' the
> > kalsyms contains address of the one from kernel/fork.c.
> >
> > So we have function model with no arguments and using it
> > to attach function with 3 arguments.. as result the trampoline
> > will not save function's arguments and we get crash because
> > trampoline changes argument registers:
> >
> >   BUG: unable to handle page fault for address: 0000607d87a1d558
> >   #PF: supervisor write access in kernel mode
> >   #PF: error_code(0x0002) - not-present page
> >   PGD 0 P4D 0
> >   Oops: 0002 [#1] SMP PTI
> >   CPU: 6 PID: 936 Comm: systemd Not tainted 5.12.0-rc4qemu+ #191
> >   RIP: 0010:mm_init+0x223/0x2a0
> >   ...
> >   Call Trace:
> >    ? bpf_trampoline_6442453476_0+0x3b/0x1000
> >    dup_mm+0x66/0x5f0
> >    ? __lock_task_sighand+0x3a/0x70
> >    copy_process+0x17d0/0x1b50
> >    kernel_clone+0x97/0x3c0
> >    __do_sys_clone+0x60/0x80
> >    do_syscall_64+0x33/0x40
> >    entry_SYSCALL_64_after_hwframe+0x44/0xae
> >   RIP: 0033:0x7f1dc9b3201f
> >
> > I think there might be more cases like this, but I don't have
> > an idea yet how to solve this in generic way. The rename in
> > this change fix it for this instance.
> 
> Just retroactively renaming functions and waiting for someone else to
> report similar issues is probably not the best strategy. Should
> resolve_btfids detect all name duplicates and emit warnings for them?
> It would be good to also remove such name-conflicting FUNCs from BTF
> (though currently it's not easy). And fail if such a function is
> referenced from .BTF_ids section.
> 
> Thoughts?

I guess we can do more checks, but I think the problem is the BTF
data vs address we get from kallsyms based on function name

we can easily get conflict address for another function with
different args

not sure how to ensure we have the proper address..  storing the
address in BTF data?

> 
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  init/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/init/main.c b/init/main.c
> > index 53b278845b88..bc1bfe57daf7 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -818,7 +818,7 @@ static void __init report_meminit(void)
> >  /*
> >   * Set up kernel memory allocators
> >   */
> > -static void __init mm_init(void)
> > +static void __init init_mem(void)
> >  {
> >         /*
> >          * page_ext requires contiguous pages,
> > @@ -905,7 +905,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> >         vfs_caches_init_early();
> >         sort_main_extable();
> >         trap_init();
> > -       mm_init();
> > +       init_mem();
> 
> nit: given trap_init and ftrace_init, mem_init probably would be a better name?

it's taken ;-)

jirka

Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

[Andrii Nakryiko]

On Tue, May 4, 2021 at 6:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:
> > On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > There are 2 mm_init functions in kernel.
> > >
> > > One in kernel/fork.c:
> > >   static struct mm_struct *mm_init(struct mm_struct *mm,
> > >                                    struct task_struct *p,
> > >                                    struct user_namespace *user_ns)
> > >
> > > And another one in init/main.c:
> > >   static void __init mm_init(void)
> > >
> > > The BTF data will get the first one, which is most likely
> > > (in my case) mm_init from init/main.c without arguments.
> > >
> > > Then in runtime when we want to attach to 'mm_init' the
> > > kalsyms contains address of the one from kernel/fork.c.
> > >
> > > So we have function model with no arguments and using it
> > > to attach function with 3 arguments.. as result the trampoline
> > > will not save function's arguments and we get crash because
> > > trampoline changes argument registers:
> > >
> > >   BUG: unable to handle page fault for address: 0000607d87a1d558
> > >   #PF: supervisor write access in kernel mode
> > >   #PF: error_code(0x0002) - not-present page
> > >   PGD 0 P4D 0
> > >   Oops: 0002 [#1] SMP PTI
> > >   CPU: 6 PID: 936 Comm: systemd Not tainted 5.12.0-rc4qemu+ #191
> > >   RIP: 0010:mm_init+0x223/0x2a0
> > >   ...
> > >   Call Trace:
> > >    ? bpf_trampoline_6442453476_0+0x3b/0x1000
> > >    dup_mm+0x66/0x5f0
> > >    ? __lock_task_sighand+0x3a/0x70
> > >    copy_process+0x17d0/0x1b50
> > >    kernel_clone+0x97/0x3c0
> > >    __do_sys_clone+0x60/0x80
> > >    do_syscall_64+0x33/0x40
> > >    entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >   RIP: 0033:0x7f1dc9b3201f
> > >
> > > I think there might be more cases like this, but I don't have
> > > an idea yet how to solve this in generic way. The rename in
> > > this change fix it for this instance.
> >
> > Just retroactively renaming functions and waiting for someone else to
> > report similar issues is probably not the best strategy. Should
> > resolve_btfids detect all name duplicates and emit warnings for them?
> > It would be good to also remove such name-conflicting FUNCs from BTF
> > (though currently it's not easy). And fail if such a function is
> > referenced from .BTF_ids section.
> >
> > Thoughts?
>
> I guess we can do more checks, but I think the problem is the BTF
> data vs address we get from kallsyms based on function name
>
> we can easily get conflict address for another function with
> different args

Assuming that BTF encodes all the functions from kallsyms, if we make
sure that there are no two FUNCs with the same name, lookup should
theoretically always return the right functions. Right? Or am I
misunderstanding something?

>
> not sure how to ensure we have the proper address..  storing the
> address in BTF data?
>
> >
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  init/main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/init/main.c b/init/main.c
> > > index 53b278845b88..bc1bfe57daf7 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -818,7 +818,7 @@ static void __init report_meminit(void)
> > >  /*
> > >   * Set up kernel memory allocators
> > >   */
> > > -static void __init mm_init(void)
> > > +static void __init init_mem(void)
> > >  {
> > >         /*
> > >          * page_ext requires contiguous pages,
> > > @@ -905,7 +905,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> > >         vfs_caches_init_early();
> > >         sort_main_extable();
> > >         trap_init();
> > > -       mm_init();
> > > +       init_mem();
> >
> > nit: given trap_init and ftrace_init, mem_init probably would be a better name?
>
> it's taken ;-)

oh, ok, never mind then

>
> jirka
>

Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

[Alexei Starovoitov]

On Tue, May 4, 2021 at 5:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, May 4, 2021 at 6:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:
> > > On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > There are 2 mm_init functions in kernel.
> > > >
> > > > One in kernel/fork.c:
> > > >   static struct mm_struct *mm_init(struct mm_struct *mm,
> > > >                                    struct task_struct *p,
> > > >                                    struct user_namespace *user_ns)
> > > >
> > > > And another one in init/main.c:
> > > >   static void __init mm_init(void)
> > > >
> > > > The BTF data will get the first one, which is most likely
> > > > (in my case) mm_init from init/main.c without arguments.

did you hack pahole in some way to get to this point?
I don't see this with pahole master.
mm_init in BTF matches the one in init/main.c. The void one.
Do you have two static mm_init-s in BTF somehow?

In general it's possible to have different static funcs with the same
name in kallsyms. I found 3 'seq_start' in my .config.
So renaming static funcs is not an option.
The simplest approach for now is to avoid emitting BTF
if there is more than one func (that will prevent attaching because
there won't be any BTF for that func).
Long term I think BTF can store the .text offset and the verifier
can avoid kallsym lookup.
We do store insn_off in bpf_func_info for bpf progs.
Something like this could be done for kernel and module funcs.
But that's long term.

Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

[Jiri Olsa]

On Tue, May 04, 2021 at 09:42:49PM -0700, Alexei Starovoitov wrote:
> On Tue, May 4, 2021 at 5:36 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, May 4, 2021 at 6:32 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:
> > > > On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > >
> > > > > There are 2 mm_init functions in kernel.
> > > > >
> > > > > One in kernel/fork.c:
> > > > >   static struct mm_struct *mm_init(struct mm_struct *mm,
> > > > >                                    struct task_struct *p,
> > > > >                                    struct user_namespace *user_ns)
> > > > >
> > > > > And another one in init/main.c:
> > > > >   static void __init mm_init(void)
> > > > >
> > > > > The BTF data will get the first one, which is most likely
> > > > > (in my case) mm_init from init/main.c without arguments.
> 
> did you hack pahole in some way to get to this point?
> I don't see this with pahole master.
> mm_init in BTF matches the one in init/main.c. The void one.
> Do you have two static mm_init-s in BTF somehow?

I have only one mm_init in BTF from init/main.c like you,
but the address in kallsyms is for the mm_init from kernel/fork.c

so we attach mm_init from kernel/fork.c with prototype from init/main.c

I'm seeing same problem also for 'receive_buf' function, which I did not post

> 
> In general it's possible to have different static funcs with the same
> name in kallsyms. I found 3 'seq_start' in my .config.
> So renaming static funcs is not an option.
> The simplest approach for now is to avoid emitting BTF
> if there is more than one func (that will prevent attaching because
> there won't be any BTF for that func).

sounds good.. will prepare the pahole change

> Long term I think BTF can store the .text offset and the verifier
> can avoid kallsym lookup.
> We do store insn_off in bpf_func_info for bpf progs.
> Something like this could be done for kernel and module funcs.
> But that's long term.
> 

ok, will check on this

jirka