Messages in this thread | | | Date | Mon, 20 Dec 2021 10:11:15 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH RESEND] random: use correct memory barriers for crng_node_pool |
| |
On Mon, Dec 20, 2021 at 04:07:28PM +0100, Jason A. Donenfeld wrote: > Hi Eric, > > This patch seems fine to me, and I'll apply it in a few days after > sitting on the list for comments, but: > > > Note: READ_ONCE() could be used instead of smp_load_acquire(), but it is > > harder to verify that it is correct, so I'd prefer not to use it here. > > (https://lore.kernel.org/lkml/20200916233042.51634-1-ebiggers@kernel.org/T/#u), > > and though it's a correct fix, it was derailed by a debate about whether > > it's safe to use READ_ONCE() instead of smp_load_acquire() or not. > > But holy smokes... I chuckled at your, "please explain in English." :) > > Paul - if you'd like to look at this patch and confirm that this > specific patch and usage is fine to be changed into READ_ONCE() > instead of smp_load_acquire(), please pipe up here. And I really do > mean this specific patch and usage, not to be confused with any other > usage elsewhere in the kernel or question about general things, which > doubtlessly involve larger discussions like the one Eric linked to > above. If you're certain this patch here is READ_ONCE()able, I'd > appreciate your saying so with a simple, "it is safe; go for it", > since I'd definitely like the optimization if it's safe. If I don't > hear from you, I'll apply this as-is from Eric, as I'd rather be safe > than sorry.
First I would want to see some evidence that READ_ONCE() was really providing measurable performance benefit. Such evidence would be easiest to obtain by running on a weakly ordered system such as ARM, ARMv8, or PowerPC.
If this does provide a measurable benefit, why not the following?
static inline struct crng_state *select_crng(void) { struct crng_state **pool; struct crng_state *pooln; int nid = numa_node_id();
/* pairs with cmpxchg_release() in do_numa_crng_init() */ pool = rcu_dereference(&crng_node_pool); if (pool) { pooln = rcu_dereference(pool[nid]); if (pooln) return pooln; }
return &primary_crng; }
This is in ignorance of the kfree() side of this code. So another question is "Suppose that there was a long delay (vCPU preemption, for example) just before the 'return pooln'. What prevents a use-after-free bug?"
Of course, this question applies equally to the smp_load_acquire() version.
Thanx, Paul
| |