Re: [PATCH 5/6] audit: Make hash table insertion safe against concurrent lookups

Jan Kara
2018-07-03T15:31:09Z

On Fri 29-06-18 16:02:10, Amir Goldstein wrote:
> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@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@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@suse.com>
SUSE Labs, CR