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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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@kernel.org>
> Suggested-by: Sebastian Sewior <bigeasy@linutronix.de>
> Fixes: 8622e45b5da1 ("uprobes: Reuse return_instances between multiple uretprobes within task")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.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