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+ctasdvzdwhza0quqqdhlfyl3d6x...@mail.gmail.com/ > Reported-by: Alexei Starovoitov <[email protected]> > Suggested-by: Sebastian Sewior <[email protected]> > Fixes: 8622e45b5da1 ("uprobes: Reuse return_instances between multiple > uretprobes within task") > Signed-off-by: Andrii Nakryiko <[email protected]> > --- > 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
