On Fri 29-06-18 16:02:10, Amir Goldstein wrote: > On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@xxxxxxx> wrote: > > Currently, the audit tree code does not make sure that when a chunk in > > inserted into the hash table, it is fully initialized. So in theory (in > > practice only on DEC Alpha) a user of RCU lookup could see uninitialized > > structure in the hash table and crash. Add appropriate barriers between > > initialization of the structure and its insertion into hash table. > > > > Signed-off-by: Jan Kara <jack@xxxxxxx> ... > > @@ -466,6 +481,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) > > tree->root = chunk; > > list_add(&tree->same_root, &chunk->trees); > > } > > + /* > > + * Make sure chunk is fully initialized before making it visible in the > > + * hash. Pairs with a data dependency barrier in READ_ONCE() in > > + * audit_tree_lookup(). > > + */ > > + smp_wmb(); > > + list_replace_rcu(&old->hash, &chunk->hash); > > IMO, now that list_replace_rcu() is no longer a one liner (including the wmb and > comment above) it would be cleaner to have a helper update_hash(old, chunk) > right next to insert_hash() and for the same reason smp_wmb with the comment > should go into insert_hash() helpler. I was thinking about this as well when writing the code. What I disliked about hiding smp_wmb() in some helper function is that after that it's much less obvious that you should have a good reason to add anything after smp_wmb() as RCU readers needn't see your write. However with some commenting, I guess it should be obvious enough. I'll do that as a separate cleanup patch though. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
