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@xxxxxxx> > > > > > > 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@xxxxxxxxxxx> > > > Suggested-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx> > > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > <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. Andrew, the fsnotify patch "fsnotify: Make fsnotify_destroy_mark_locked() safe without refcount" will stop applying after the revert happens. Just drop it. I will send an updated version later as after spending a day in fsnotify code I think I finally see a way how to get rid of fsnotify_destroy_mark_locked() which is just too error-prone to use. But that definitely isn't for this cycle. Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
