On Thu, Jan 09, 2025 at 03:41:26PM +0100, Daniel Borkmann wrote:
> On 1/9/25 3:14 PM, Jiri Olsa wrote:
> > Max Makarov reported kernel panic [1] in perf user callchain code.
> >
> > The reason for that is the race between uprobe_free_utask and bpf
> > profiler code doing the perf user stack unwind and is triggered
> > within uprobe_free_utask function:
> > - after current->utask is freed and
> > - before current->utask is set to NULL
> >
> > general protection fault, probably for non-canonical address 0x9e759c37ee555c76: 0000 [#1] SMP PTI
> > RIP: 0010:is_uprobe_at_func_entry+0x28/0x80
> > ...
> > ? die_addr+0x36/0x90
> > ? exc_general_protection+0x217/0x420
> > ? asm_exc_general_protection+0x26/0x30
> > ? is_uprobe_at_func_entry+0x28/0x80
> > perf_callchain_user+0x20a/0x360
> > get_perf_callchain+0x147/0x1d0
> > bpf_get_stackid+0x60/0x90
> > bpf_prog_9aac297fb833e2f5_do_perf_event+0x434/0x53b
> > ? __smp_call_single_queue+0xad/0x120
> > bpf_overflow_handler+0x75/0x110
> > ...
> > asm_sysvec_apic_timer_interrupt+0x1a/0x20
> > RIP: 0010:__kmem_cache_free+0x1cb/0x350
> > ...
> > ? uprobe_free_utask+0x62/0x80
> > ? acct_collect+0x4c/0x220
> > uprobe_free_utask+0x62/0x80
> > mm_release+0x12/0xb0
> > do_exit+0x26b/0xaa0
> > __x64_sys_exit+0x1b/0x20
> > do_syscall_64+0x5a/0x80
> >
> > It can be easily reproduced by running following commands in
> > separate terminals:
> >
> > # while :; do bpftrace -e 'uprobe:/bin/ls:_start { printf("hit\n"); }' -c ls; done
> > # bpftrace -e 'profile:hz:100000 { @[ustack()] = count(); }'
> >
> > Fixing this by making sure current->utask pointer is set to NULL
> > before we start to release the utask object.
>
> Lets add Fixes tag for stable team:
>
> Fixes: cfa7f3d2c526 ("perf,x86: avoid missing caller address in stack traces captured in uprobe")
ugh right, thanks for finding that
jirka
>
> > [1] https://github.com/grafana/pyroscope/issues/3673
> > Reported-by: Max Makarov <maxpain@xxxxxxxxx>
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>
> fwiw, the other version we were potentially thinking of was below, but
> just moving the t->utask NULL assignment seemed better.
>
> Thanks,
> Daniel
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index c75c482d4c52..05f9cedf2691 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2835,6 +2835,8 @@ static bool is_uprobe_at_func_entry(struct pt_regs *regs)
>
> if (!current->utask)
> return false;
> + if (!current->utask->active_uprobe)
> + return false;
>
> auprobe = current->utask->auprobe;
> if (!auprobe)
