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

[Ingo Molnar <mingo@kernel.org>]

* Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Thu, Apr 3, 2025 at 10:49 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > 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.
> 
> yep, will fix; no, uprobe is not networking, this style is just
> ingrained in my brain from working in BPF code base for a while

... and this example underlines why we've been asking the networking 
folks for years to use the standard Linux kernel coding style for 
comments, instead of creating this pointless noise & inconsistency.

Thanks,

	Ingo