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