Cmnt: [SRU][N/O][PATCH 1/1] uprobes: Fix race in uprobe_free_utask

[Koichiro Den]

On Tue, Mar 25, 2025 at 05:52:03PM GMT, Krister Johansen wrote:
> From: Jiri Olsa <jolsa at kernel.org>
> 
> BugLink: https://bugs.launchpad.net/bugs/2104210
> 
> commit b583ef82b671c9a752fbe3e95bd4c1c51eab764d upstream.
> 
> 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.
> 
> [1] https://github.com/grafana/pyroscope/issues/3673
> 
> Fixes: cfa7f3d2c526 ("perf,x86: avoid missing caller address in stack traces captured in uprobe")
> Reported-by: Max Makarov <maxpain at linux.com>
> Signed-off-by: Jiri Olsa <jolsa at kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz at infradead.org>
> Acked-by: Oleg Nesterov <oleg at redhat.com>
> Acked-by: Andrii Nakryiko <andrii at kernel.org>
> Link: https://lore.kernel.org/r/20250109141440.2692173-1-jolsa@kernel.org
> [Christian Simon: Rebased for 6.12.y, due to mainline change https://lore.kernel.org/all/20240929144239.GA9475@redhat.com/]
> Signed-off-by: Christian Simon <simon at swine.de>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry-picked from stable-6.6.y eff00c5e29abfc919a0647cb85c5ac6a5d6e1f0a)

Thanks for the submission and for fixing the issue for Ubuntu so quickly.

LGTM except one trivial formatting issue:
could you please re-send with the provenance corrected as follows?

  (cherry picked from commit eff00c5e29abfc919a0647cb85c5ac6a5d6e1f0a linux-6.6.y)

Ref. https://canonical-kernel-docs.readthedocs-hosted.com/en/latest/reference/stable-patch-format/#comment-body

Thanks.

> Signed-off-by: Krister Johansen <kjlx at templeofstupid.com>
> ---
>  kernel/events/uprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 1c118259e8b3..4abdee6f6a96 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1716,6 +1716,7 @@ void uprobe_free_utask(struct task_struct *t)
>  	if (!utask)
>  		return;
>  
> +	t->utask = NULL;
>  	if (utask->active_uprobe)
>  		put_uprobe(utask->active_uprobe);
>  
> @@ -1725,7 +1726,6 @@ void uprobe_free_utask(struct task_struct *t)
>  
>  	xol_free_insn_slot(t);
>  	kfree(utask);
> -	t->utask = NULL;
>  }
>  
>  /*
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team