linux-kernel - Re: [PATCH tip/perf] uprobes: avoid false lockdep splat in uprobe timer callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250403174917.OLHfwBp-@linutronix.de>
Date: Thu, 3 Apr 2025 19:49:17 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Andrii Nakryiko <andrii@...nel.org>
Cc: linux-trace-kernel@...r.kernel.org, peterz@...radead.org,
	mingo@...nel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-team@...a.com, rostedt@...dmis.org, oleg@...hat.com,
	mhiramat@...nel.org, ast@...nel.org
Subject: Re: [PATCH tip/perf] uprobes: avoid false lockdep splat in uprobe
 timer callback

On 2025-04-03 10:18:31 [-0700], Andrii Nakryiko wrote:
> Avoid a false-positive lockdep warning in PREEMPT_RT configuration when
> using write_seqcount_begin() in uprobe timer callback by using
> raw_write_* APIs. Uprobe's use of timer callback is guaranteed to not
> race with itself, and as such seqcount's insistence on having hardirqs
preemption, not hardirqs

> disabled on the writer side is irrelevant. So switch to raw_ variants of
> seqcount API instead of disabling hardirqs unnecessarily.
> 
> Also, point out in the comments more explicitly why we use seqcount
> despite our reader side being rather simple and never retrying. We favor
> well-maintained kernel primitive in favor of open-coding our own memory
> barriers.

Thank you.

> Link: https://lore.kernel.org/bpf/CAADnVQLLOHZmPO4X_dQ+cTaSDvzdWHzA0qUqQDhLFYL3D6xPxg@mail.gmail.com/
> Reported-by: Alexei Starovoitov <ast@...nel.org>
> Suggested-by: Sebastian Sewior <bigeasy@...utronix.de>
> Fixes: 8622e45b5da1 ("uprobes: Reuse return_instances between multiple uretprobes within task")
> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> ---
>  kernel/events/uprobes.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 70c84b9d7be3..6d7e7da0fbbc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1944,6 +1944,9 @@ static void free_ret_instance(struct uprobe_task *utask,
>  	 * to-be-reused return instances for future uretprobes. If ri_timer()
>  	 * happens to be running right now, though, we fallback to safety and
>  	 * just perform RCU-delated freeing of ri.
> +	 * Admittedly, this is a rather simple use of seqcount, but it nicely
> +	 * abstracts away all the necessary memory barriers, so we use
> +	 * a well-supported kernel primitive here.
>  	 */
>  	if (raw_seqcount_try_begin(&utask->ri_seqcount, seq)) {
>  		/* immediate reuse of ri without RCU GP is OK */
> @@ -2004,12 +2007,18 @@ static void ri_timer(struct timer_list *timer)
>  	/* RCU protects return_instance from freeing. */
>  	guard(rcu)();
>  
> -	write_seqcount_begin(&utask->ri_seqcount);

> +	/* See free_ret_instance() for notes on seqcount use.

This is not a proper multi line comment.

> +	 * We also employ raw API variants to avoid lockdep false-positive
> +	 * warning complaining about hardirqs not being disabled. We have

s/hardirqs/preemption. The warning is about missing disabled preemption.

> +	 * a guarantee that this timer callback won't race with itself, so no
> +	 * need to disable hardirqs.

The timer can only be invoked once for a uprobe_task. Therefore there
can only be one writer. The reader does not require an even sequence
count so it is okay to remain preemptible on PREEMPT_RT. 

> +	 */
> +	raw_write_seqcount_begin(&utask->ri_seqcount);
>  
>  	for_each_ret_instance_rcu(ri, utask->return_instances)
>  		hprobe_expire(&ri->hprobe, false);
>  
> -	write_seqcount_end(&utask->ri_seqcount);
> +	raw_write_seqcount_end(&utask->ri_seqcount);
>  }
>  
>  static struct uprobe_task *alloc_utask(void)

Sebastian