From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 1/3] fsnotify: Fix oops in fsnotify_clear_marks_by_group_flags() Date: Tue, 21 Jul 2015 22:35:22 +0200 Message-ID: <20150721203522.GA10985@quack.suse.cz> References: <1436966481-12517-1-git-send-email-jack@suse.com> <1436966481-12517-2-git-send-email-jack@suse.com> <55AB7A3D.4000008@gmail.com> <20150720144642.GE3131@quack.suse.cz> <20150721200339.GA12971@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , Kinglong Mee , linux-fsdevel@vger.kernel.org, Jan Kara To: Linus Torvalds , Andrew Morton Return-path: Received: from cantor2.suse.de ([195.135.220.15]:59947 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932116AbbGUUf2 (ORCPT ); Tue, 21 Jul 2015 16:35:28 -0400 Content-Disposition: inline In-Reply-To: <20150721200339.GA12971@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > > > > > > > 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 > > > > Suggested-by: Lino Sanfilippo > > > > Signed-off-by: Jan Kara > > > > > > > > > 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: > > > [] kmemleak_alloc+0x4e/0xb0 > > > [] __kmalloc+0x1e5/0x290 > > > [] inotify_handle_event+0x75/0x160 > > > [] inotify_ignored_and_remove_idr+0x5c/0x80 > > > [] inotify_freeing_mark+0xe/0x10 > > > [] fsnotify_mark_destroy+0xb6/0x150 > > > [] kthread+0xd7/0xf0 > > > [] ret_from_fork+0x3f/0x70 > > > [] 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 SUSE Labs, CR