Re: [PATCH 02/10] audit: Fix possible spurious -ENOSPC error

[Jan Kara <jack@suse.cz>]

On Fri 27-07-18 00:47:10, Paul Moore wrote:
> On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack@suse.cz> 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 <jack@suse.cz>
> > ---
> >  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 <jack@suse.com>
SUSE Labs, CR