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

[Jan Kara <jack@suse.cz>]

On Tue 21-07-15 22:03:39, Jan Kara wrote:
> 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.
> 
> OK, I have looked into the code some more and I found another relatively
> simple way of fixing the original oops. It will be IMHO better than trying
> to fixup this issue which has more potential for breakage. I'll ask Linus
> to revert the fsnotify fix he already merged and send a new fix.

Linus, please revert commit

a2673b6e0406 fsnotify: fix oops in fsnotify_clear_marks_by_group_flags()

It fixes the oops but introduces a race which can leak event structure on
group destruction. I'll fix the oops in a different way and it would
basically undo what this patch did anyway. Thanks!

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