From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 5/6] audit: Make hash table insertion safe against concurrent lookups Date: Tue, 3 Jul 2018 17:31:09 +0200 Message-ID: <20180703153109.2krilssfevbva57x@quack2.suse.cz> References: <20180628164014.4925-1-jack@suse.cz> <20180628164014.4925-6-jack@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Amir Goldstein Cc: Jan Kara , Richard Guy Briggs , Linux Audit , Al Viro , linux-fsdevel List-Id: linux-audit@redhat.com On Fri 29-06-18 16:02:10, Amir Goldstein wrote: > On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara 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 ... > > @@ -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 SUSE Labs, CR