On Tue, May 4, 2021 at 6:32 AM Jiri Olsa [off-list ref] wrote:

On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:

quoted

On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa [off-list ref] wrote:

quoted

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?

quoted

quoted

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