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. 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
