Re: [PATCH RESEND] random: use correct memory barriers for crng_node_pool

["Paul E. McKenney" <paulmck@kernel.org>]

On Mon, Dec 20, 2021 at 12:35:05PM -0600, Eric Biggers wrote:
> On Mon, Dec 20, 2021 at 10:31:40AM -0800, Paul E. McKenney wrote:
> > On Mon, Dec 20, 2021 at 07:16:48PM +0100, Jason A. Donenfeld wrote:
> > > On Mon, Dec 20, 2021 at 7:11 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > First I would want
> > > 
> > > It looks like you've answered my question with four other questions,
> > > which seem certainly technically warranted, but also indicates we're
> > > probably not going to get to the nice easy resting place of, "it is
> > > safe; go for it" that I was hoping for. In light of that, it seems
> > > like merging Eric's patch is reasonable.
> > 
> > My hope would be that the questions can be quickly answered by the
> > developers and maintainers.  But yes, hope springs eternal.
> > 
> > 							Thanx, Paul
> 
> I wouldn't expect READ_ONCE() to provide a noticable performance improvement
> here, as it would be lost in the noise of the other work done, especially
> chacha20_block().
> 
> The data structures in question are never freed, so your other questions are
> irrelevant, if I understand correctly.

Very good, and thank you!  You are correct, if the structures never are
freed, there is no use-after-free issue.  And that also explains why I
was not able to find the free path.  ;-)

So the main issue is the race between insertion and lookup.  So yes,
READ_ONCE() suffices.

This assumes that the various crng_node_pool[i] pointers never change
while accessible to readers (and that some sort of synchronization applies
to the values in the pointed-to structure).  If these pointers do change,
then there also needs to be a READ_ONCE(pool[nid]) in select_crng(), where
the value returned from this READ_ONCE() is both tested and returned.
(As in assign this value to a temporary.)

But if the various crng_node_pool[i] pointers really are constant
while readers can access them, then the cmpxchg_release() suffices.
The loads from pool[nid] are then data-race free, and because they
are unmarked, the compiler is prohibited from hoisting them out from
within the "if" statement.  The address dependency prohibits the
CPU from reordering them.

So READ_ONCE() should be just fine.  Which answers Jason's question.  ;-)

Looking at _extract_crng(), if this was my code, I would use READ_ONCE()
in the checks, but that might be my misunderstanding boot-time constraints
or some such.  Without some sort of constraint, I don't see how the code
avoids confusion from reloads of crng->init_time if two CPUs concurrently
see the expiration of CRNG_RESEED_INTERVAL, but I could easily be missing
something that makes this safe.  (And this is irrelevant to this patch.)

You do appear to have ->lock guarding the pointed-to data, so that
is good.

							Thanx, Paul