lkml.org 
[lkml]   [2017]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] random: silence compiler warnings and fix race
On 2017-06-19 22:55:37 [+0200], Jason A. Donenfeld wrote:
> On Mon, Jun 19, 2017 at 9:45 AM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > ehm. You sure? I simply delayed the lock-dropping _after_ the state
> > variable was been modified. So it was basically what your patch did
> > except it was unlocked later…
>
> Yes, I'm sure. You moved the call to invalidate_batched_entropy() to
> be after the assignment of crng_init. However, the call to
> invalidate_batched_entropy() must be made _before_ the assignment of
> crng_init.

so you need to find a another way then. Doing the assignment after
dropping the lock opens another race.

> >> > Are use about that? I am not sure that the gcc will inline "crng_init"
> >> > read twice. It is not a local variable. READ_ONCE() is usually used
> >> > where gcc could cache a memory access but you do not want this. But hey!
> >> > If someone knows better I am here to learn.
> >>
> >> The whole purpose is that I _want_ it to cache the memory access so
> >> that it is _not_ inlined. So, based on your understanding, it does
> >> exactly what I intended it to do. The reason is that I'd like to avoid
> >> a lock imbalance, which could happen if the read is inlined.
> >
> > So it was good as it was which means you can drop that READ_ONCE().
>
> Except READ_ONCE ensures that the compiler will never inline it, so it
> actually needs to stay.

I don't think the compiler is allowed to inline it the way you describe
it. This would render any assignment to local variable useless. Also
the READ_ONCE creates worse code in this case (because the read can not
be delayed).

Sebastian

\
 
 \ /
  Last update: 2017-06-20 08:45    [W:0.122 / U:1.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site