archive mirror
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <>
To: Eric Biggers <>
Cc: "Jason A. Donenfeld" <>,
	Theodore Ts'o <>,
	LKML <>,
	Linux Crypto Mailing List <>,
	stable <>
Subject: Re: [PATCH RESEND] random: use correct memory barriers for crng_node_pool
Date: Mon, 20 Dec 2021 11:00:04 -0800	[thread overview]
Message-ID: <20211220190004.GD641268@paulmck-ThinkPad-P17-Gen-1> (raw)
In-Reply-To: <YcDM2cpwiGCb56Gp@quark>

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 <> 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

  reply	other threads:[~2021-12-20 19:00 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-19  2:51 [PATCH RESEND] random: use correct memory barriers for crng_node_pool Eric Biggers
2021-12-20 15:07 ` Jason A. Donenfeld
2021-12-20 18:11   ` Paul E. McKenney
2021-12-20 18:16     ` Jason A. Donenfeld
2021-12-20 18:31       ` Paul E. McKenney
2021-12-20 18:35         ` Eric Biggers
2021-12-20 19:00           ` Paul E. McKenney [this message]
2021-12-20 21:45             ` Jason A. Donenfeld
2021-12-20 22:10               ` Eric Biggers
2021-12-20 15:17 ` Jason A. Donenfeld
2021-12-20 15:38   ` Eric Biggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211220190004.GD641268@paulmck-ThinkPad-P17-Gen-1 \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).