Re: uprobe splat in PREEMP_RT

Andrii Nakryiko
2025-04-02T16:15:27Z

On Wed, Apr 2, 2025 at 3:34 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 04/01, Andrii Nakryiko wrote:
> >
> > On Tue, Apr 1, 2025 at 2:21 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > Hmm,
> > >
> > > 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);
> > >
> > > How big can that loop be?
> > >
> > > Of course, we could just say not to use uprobes on PREEMPT_RT kernels?
> > > Otherwise, they could cause an unspecified latency.
> >
> > There can't be more than 64 nested uretprobes, so it will be (in a
> > very-very unlikely event) at most 64 items. And that hprobe_expire()
> > operation is very fast. So I don't think latency is a big concert
> > here.
>
> I still didn't read this code, but after the quick glance I don't
> understand why do we actually need utask->ri_seqcount.
>
> The "writer" ri_timer() can't race with itself, right?
>
> The "reader" free_ret_instance() uses raw_seqcount_try_begin() without
> the "retry" logic.
>
> I have no idea if this logic is correct or not, but it seems that (apart
> from the necessary barriers) we could use the utask->ri_timer_is_running
> boolean instead with the same effect? Set/cleared in ri_timer(), checked
> in free_ret_instance().

"Apart from the necessary barriers" is exactly what I didn't want to
deal with, tbh... Which is why I went with (ab)using seqcount lock.

Other than that, yes, the reader logic is very simple and just wants
to make sure that ri_timer (writer) couldn't have seen the
return_instance we are about to immediately reuse (which would pose a
problem).

>
> I must have missed something...
>
> Oleg.
>