Re: [PATCH] uprobes: Fix race in uprobe_free_utask

[Jiri Olsa]

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)