On Thu, Apr 3, 2025 at 10:49 AM Sebastian Andrzej Siewior <[email protected]> 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+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.
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
