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