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: <CAEf4BzasxUn+Ywi-=TtP+S+i4VBLnKvYCkxPCz63o4zEXT9QZw@mail.gmail.com>
Date: Thu, 3 Apr 2025 11:30:42 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Andrii Nakryiko <andrii@...nel.org>, 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 Thu, Apr 3, 2025 at 10:49 AM Sebastian Andrzej Siewior
<bigeasy@...utronix.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@...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.

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

>
> > +      * 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.

Right, sorry, the `this_cpu_read(hardirqs_enabled)` part of the check
in lockdep_assert_preemption_disabled() made too strong an impression
on me :) Will fix.

>
> > +      * 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