On Fri, Aug 21, 2020 at 08:30:43AM +0200, Marco Elver wrote:
> With KCSAN enabled, prandom_u32() may be called from any context,
> including idle CPUs.
>
> Therefore, switch to using trace_prandom_u32_rcuidle(), to avoid various
> issues due to recursion and lockdep warnings when KCSAN and tracing is
> enabled.
At some point we're going to have to introduce noinstr to idle as well.
But until that time this should indeed cure things.
On Fri, Aug 21, 2020 at 1:59 AM <[email protected]> wrote:
>
> On Fri, Aug 21, 2020 at 08:30:43AM +0200, Marco Elver wrote:
> > With KCSAN enabled, prandom_u32() may be called from any context,
> > including idle CPUs.
> >
> > Therefore, switch to using trace_prandom_u32_rcuidle(), to avoid various
> > issues due to recursion and lockdep warnings when KCSAN and tracing is
> > enabled.
>
> At some point we're going to have to introduce noinstr to idle as well.
> But until that time this should indeed cure things.
I do not understand what the issue is. This _rcuidle() is kind of opaque ;)
Would this alternative patch work, or is it something more fundamental ?
Thanks !
diff --git a/lib/random32.c b/lib/random32.c
index 932345323af092a93fc2690b0ebbf4f7485ae4f3..17af2d1631e5ab6e02ad1e9288af7e007bed6d5f
100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -83,9 +83,10 @@ u32 prandom_u32(void)
u32 res;
res = prandom_u32_state(state);
- trace_prandom_u32(res);
put_cpu_var(net_rand_state);
+ trace_prandom_u32(res);
+
return res;
}
EXPORT_SYMBOL(prandom_u32);
On Fri, 21 Aug 2020 08:06:49 -0700
Eric Dumazet <[email protected]> wrote:
> On Fri, Aug 21, 2020 at 1:59 AM <[email protected]> wrote:
> >
> > On Fri, Aug 21, 2020 at 08:30:43AM +0200, Marco Elver wrote:
> > > With KCSAN enabled, prandom_u32() may be called from any context,
> > > including idle CPUs.
> > >
> > > Therefore, switch to using trace_prandom_u32_rcuidle(), to avoid various
> > > issues due to recursion and lockdep warnings when KCSAN and tracing is
> > > enabled.
> >
> > At some point we're going to have to introduce noinstr to idle as well.
> > But until that time this should indeed cure things.
>
> I do not understand what the issue is. This _rcuidle() is kind of opaque ;)
kasan can be called when RCU is not "watching". That is, in the idle
code, RCU will stop bothering idle CPUs by checking on them to move
along the grace period. Just before going to idle, RCU will just set
that its in a quiescent state. The issue is, after RCU has basically
shutdown, and before getting to where the CPU is "sleeping", kasan is
called, and kasan call a tracepoint. The problem is that tracepoints
are protected by RCU. If RCU has shutdown, then it loses the
protection. There's code to detect this and give a warning.
All tracepoints have a _rcuidle() version. What this does is adds a
little bit more overhead to the tracepoint when enabled to check if RCU
is watching or not. If it is not watching, it tells RCU to start
watching again while it runs the tracepoint, and afterward it lets RCU
know that it can go back to not watching.
>
> Would this alternative patch work, or is it something more fundamental ?
As I hope the above explained. The answer is "no".
-- Steve
>
> Thanks !
>
> diff --git a/lib/random32.c b/lib/random32.c
> index 932345323af092a93fc2690b0ebbf4f7485ae4f3..17af2d1631e5ab6e02ad1e9288af7e007bed6d5f
> 100644
> --- a/lib/random32.c
> +++ b/lib/random32.c
> @@ -83,9 +83,10 @@ u32 prandom_u32(void)
> u32 res;
>
> res = prandom_u32_state(state);
> - trace_prandom_u32(res);
> put_cpu_var(net_rand_state);
>
> + trace_prandom_u32(res);
> +
> return res;
> }
> EXPORT_SYMBOL(prandom_u32);
On Fri, 21 Aug 2020 11:38:31 -0400
Steven Rostedt <[email protected]> wrote:
> > > At some point we're going to have to introduce noinstr to idle as well.
> > > But until that time this should indeed cure things.
> >
What the above means, is that ideally we will get rid of all
tracepoints and kasan checks from these RCU not watching locations. But
to do so, we need to move the RCU not watching as close as possible to
where it doesn't need to be watching, and that is not as trivial of a
task as one might think. Once we get to a minimal code path for RCU not
to be watching, it will become "noinstr" and tracing and "debugging"
will be disabled in these sections.
Peter, please correct the above if it's not accurate.
-- Steve
On Fri, Aug 21, 2020 at 11:41:41AM -0400, Steven Rostedt wrote:
> On Fri, 21 Aug 2020 11:38:31 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > > > At some point we're going to have to introduce noinstr to idle as well.
> > > > But until that time this should indeed cure things.
> > >
>
> What the above means, is that ideally we will get rid of all
> tracepoints and kasan checks from these RCU not watching locations. But
s/and kasan//
We only need to get rid of explicit tracepoints -- typically just move
them outside of the rcu_idle_enter/exit section.
> to do so, we need to move the RCU not watching as close as possible to
> where it doesn't need to be watching, and that is not as trivial of a
> task as one might think.
My recent patch series got a fair way towards that, but yes, there's
more work to be done still.
> Once we get to a minimal code path for RCU not
> to be watching, it will become "noinstr" and tracing and "debugging"
> will be disabled in these sections.
Right, noinstr is like notrace on steriods, not only does it disallow
tracing, it will also disable all the various *SAN/KCOV instrumentation.
It also has objtool based validation which ensures noinstr code doesn't
call out to regular code.