Re: [PATCH 1/3] fsnotify: Fix oops in fsnotify_clear_marks_by_group_flags()

[Jan Kara <jack@suse.cz>]

On Mon 20-07-15 16:46:42, Jan Kara wrote:
> On Sun 19-07-15 18:21:49, Kinglong Mee wrote:
> > On 7/15/2015 21:21, Jan Kara wrote:
> > > From: Jan Kara <jack@suse.cz>
> > > 
> > > fsnotify_clear_marks_by_group_flags() can race with
> > > fsnotify_destroy_marks() so when fsnotify_destroy_mark_locked() drops
> > > mark_mutex, a mark from the list iterated by
> > > fsnotify_clear_marks_by_group_flags() can be freed and we dereference
> > > free memory in the loop there.
> > > 
> > > Fix the problem by keeping mark_mutex held in
> > > fsnotify_destroy_mark_locked(). The reason why we drop that mutex is
> > > that we need to call a ->freeing_mark() callback which may acquire
> > > mark_mutex again. To avoid this and similar lock inversion issues, we
> > > move the call to ->freeing_mark() callback to the kthread destroying the
> > > mark.
> > > 
> > > Reported-by: Ashish Sangwan <a.sangwan@samsung.com>
> > > Suggested-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> <snip>
> 
> > With this patch, I got so many memleak notice, 
> > 
> > unreferenced object 0xffff880035bef640 (size 64):
> >   comm "fsnotify_mark", pid 26, jiffies 4294673717 (age 628.737s)
> >   hex dump (first 32 bytes):
> >     28 36 3f 76 00 88 ff ff 28 36 3f 76 00 88 ff ff  (6?v....(6?v....
> >     00 00 00 00 00 00 00 00 00 80 00 00 00 00 ad de  ................
> >   backtrace:
> >     [<ffffffff816cd34e>] kmemleak_alloc+0x4e/0xb0
> >     [<ffffffff811ac6b5>] __kmalloc+0x1e5/0x290
> >     [<ffffffff81204f25>] inotify_handle_event+0x75/0x160
> >     [<ffffffff81205abc>] inotify_ignored_and_remove_idr+0x5c/0x80
> >     [<ffffffff8120505e>] inotify_freeing_mark+0xe/0x10
> >     [<ffffffff81203ca6>] fsnotify_mark_destroy+0xb6/0x150
> >     [<ffffffff810a4487>] kthread+0xd7/0xf0
> >     [<ffffffff816d92df>] ret_from_fork+0x3f/0x70
> >     [<ffffffffffffffff>] 0xffffffffffffffff
> > 
> > It is caused by ->freeing_mark() insert an event to group,
> > but snotify_put_mark() kfree the group without free the event.
> 
> Thanks for report! You are right that my patch introduces a race between
> fsnotify kthread and fsnotify_destroy_group() which can result in leaking
> inotify event on group destruction. I haven't yet decided whether the right
> fix is not to queue events for dying notification group (as that is
> pointless anyway) or whether we should just fix the original problem
> differently... Whenever I look at fsnotify code mark handling I get lost in
> the maze of locks, lists, and subtle differences between how different
> notification systems handle notification marks :( I'll think about it over
> night.

Bah, my sent emails were just getting queued for last two days. Sorry for
the delay..

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR