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