Re: [PATCH 5/6] audit: Make hash table insertion safe against concurrent lookups
On Fri 29-06-18 16:02:10, Amir Goldstein wrote:
On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack(a)suse.cz>
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(a)suse.cz>
...
> @@ -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(a)suse.com>
SUSE Labs, CR