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

[Jan Kara <jack@suse.cz>]

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.

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