From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH] fsnotify: fix a crash due to invalid virtual address Date: Wed, 24 Jun 2015 10:42:24 +0200 Message-ID: <20150624084224.GA17849@quack.suse.cz> References: <1434970396-19644-1-git-send-email-a.sangwan@samsung.com> <20150623102521.GD2427@quack.suse.cz> <5589DDF8.6060406@gmx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , Ashish Sangwan , linux-fsdevel@vger.kernel.org, Andrew Morton , Eric Paris , Amit Sahrawat , Namjae Jeon , Pankaj Mishra To: Lino Sanfilippo Return-path: Received: from cantor2.suse.de ([195.135.220.15]:46403 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750847AbbFXImb (ORCPT ); Wed, 24 Jun 2015 04:42:31 -0400 Content-Disposition: inline In-Reply-To: <5589DDF8.6060406@gmx.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi, On Wed 24-06-15 00:30:16, Lino Sanfilippo wrote: > On 23.06.2015 12:25, Jan Kara wrote: > > On Mon 22-06-15 16:23:16, Ashish Sangwan wrote: > >> For deleting the fsnotify_mark related with an inode, there are 2 paths in the > >> kernel. When the inotify fd is closed, all the marks belonging to a group are > >> removed one by one in fsnotify_clear_marks_by_group_flags. Other path is when > >> the inode is removed from user space by unlink, fsnotify_destroy_mark is > >> called to delete a single mark. > >> There is a race between these 2 paths which is caused due to the temporary > >> release of the mark_mutex inside fsnotify_destroy_mark_locked. > >> The race happen when the inotify app monitoring the file(s) exits, triggering > >> fsnotify_clear_marks_by_group_flags to delete the marks. > >> This function use lmark pointer to move to the next node after a safe removal > >> of the node. In parallel, if there is rm call for a file and such that the > >> lmark is pointing to the mark which is removed by this rm call, lmark ends up > >> pointing to a freed memory. Now, when we try to move to the next node using > >> lmark, it triggers an invalid virtual address crash. > >> Although fsnotify_clear_marks_by_group_flags and fsnotify_destroy_mark are > >> synchronized by mark_mutex, but both of these functions call > >> fsnotify_destroy_mark_locked which release the mark_mutex and acquire it again > >> creating a subtle race window. There seems to be no reason for releasing > >> mark_mutex, so this patch remove the mutex_unlock call. > > > > Thanks for report and the analysis. I agree with your problem analysis. > > Indeed the loop in fsnotify_clear_marks_by_group_flags() isn't safe against > > us dropping the mark_mutex inside fsnotify_destroy_mark_locked(). However > > mark_mutex is dropped in fsnotify_destroy_mark_locked() for a purpose. We > > call ->freeing_mark() callback from there and that should be called without > > mark_mutex. In particular inotify uses this callback to send the IN_IGNORE > > event and that code certainly isn't prepared to be called under mark_mutex > > and you likely introduce interesting deadlock possibilities there. > > Why dont we call freeing_mark() from the "fsnotify_mark"-thread instead > of fsnotify_destroy_mark_locked()? So there would not be a reason for > this temporary unlock any longer and we could close that race as Ashish > suggested. We could do that as well. But I'd prefer to keep sending the IN_IGNORED event from the context of the process destroying the mark (not that I would be aware of any strong reason why that must happen but it just seems more natural). Also the event from destruction thread will be sent with a delay caused by synchronize_srcu(). Finally one long critical section for destruction of all marks belonging to a group doesn't seem ideal either. Anyway, I'll have this possibility in mind when implementing some solution. Maybe it will be the most elegant way... Honza -- Jan Kara SUSE Labs, CR