From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lino Sanfilippo Subject: Re: [PATCH] fsnotify: fix a crash due to invalid virtual address Date: Wed, 24 Jun 2015 00:30:16 +0200 Message-ID: <5589DDF8.6060406@gmx.de> References: <1434970396-19644-1-git-send-email-a.sangwan@samsung.com> <20150623102521.GD2427@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, Andrew Morton , Eric Paris , Amit Sahrawat , Namjae Jeon , Pankaj Mishra To: Jan Kara , Ashish Sangwan Return-path: Received: from mout.gmx.net ([212.227.15.15]:49678 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752275AbbFWWa1 (ORCPT ); Tue, 23 Jun 2015 18:30:27 -0400 In-Reply-To: <20150623102521.GD2427@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi, 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. Lino