From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:35318 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726087AbeIDOYd (ORCPT ); Tue, 4 Sep 2018 10:24:33 -0400 Date: Tue, 4 Sep 2018 12:00:08 +0200 From: Jan Kara To: Paul Moore Cc: jack@suse.cz, linux-audit@redhat.com, linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, rgb@redhat.com, amir73il@gmail.com Subject: Re: [PATCH 02/10] audit: Fix possible spurious -ENOSPC error Message-ID: <20180904100008.GF9444@quack2.suse.cz> References: <20180710100217.12866-1-jack@suse.cz> <20180710100217.12866-3-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri 27-07-18 00:47:10, Paul Moore wrote: > On Tue, Jul 10, 2018 at 6:02 AM Jan Kara wrote: > > When an inode is tagged with a tree, tag_chunk() checks whether there is > > audit_tree_group mark attached to the inode and adds one if not. However > > nothing protects another tag_chunk() to add the mark between we've > > checked and try to add the fsnotify mark thus resulting in an error from > > fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk(). > > > > Fix the problem by holding mark_mutex over the whole check-insert code > > sequence. > > > > Signed-off-by: Jan Kara > > --- > > kernel/audit_tree.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > ... > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 1c82eb6674c4..de8d344d91b1 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -342,25 +342,29 @@ static void untag_chunk(struct node *p) > > spin_lock(&hash_lock); > > } > > > > +/* Call with group->mark_mutex held, releases it */ > > Stuff like that always makes me nervous. Yes, I also prefer to avoid stuff like this. > Could we defer releasing the mutex to the caller, after create_chunk() > returns? It looks like fsnotify_destroy_mark() allows a single level of > nesting so it should be okay, yes? This won't work. fsnotify_destroy_mark() would try to acquire the same mutex and block indefinitely (the nesting depth is there just for lockdep so that you can possibly nest mark_mutexes of two different group's). And even if we do more work and use separate fsnotify_detach_mark() and fsnotify_free_mark() calls instead of fsnotify_destroy_mark(), the problem is still there as fsnotify_free_mark() must not be called under mark_mutex (as it can acquire it in some cases). So as much as I don't like functions that release locks they didn't take I don't see how to avoid that here without creating even bigger mess. Honza -- Jan Kara SUSE Labs, CR