From patchwork Wed Jun 24 10:13:50 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ashish Sangwan X-Patchwork-Id: 6666661 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 15988C05AC for ; Wed, 24 Jun 2015 10:14:56 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 71F5A2051F for ; Wed, 24 Jun 2015 10:14:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 21A6B205F4 for ; Wed, 24 Jun 2015 10:14:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752050AbbFXKOv (ORCPT ); Wed, 24 Jun 2015 06:14:51 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:37001 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750974AbbFXKOu (ORCPT ); Wed, 24 Jun 2015 06:14:50 -0400 Received: from epcpsbgr4.samsung.com (u144.gpu120.samsung.co.kr [203.254.230.144]) by mailout2.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NQG00B381SO6S90@mailout2.samsung.com> for linux-fsdevel@vger.kernel.org; Wed, 24 Jun 2015 19:14:48 +0900 (KST) Received: from epcpsbgx4.samsung.com ( [172.20.52.125]) by epcpsbgr4.samsung.com (EPCPMTA) with SMTP id FF.73.20564.8138A855; Wed, 24 Jun 2015 19:14:48 +0900 (KST) X-AuditID: cbfee690-f796f6d000005054-8c-558a8318a2e5 Received: from epmailer02 ( [203.254.219.142]) by epcpsbgx4.samsung.com (EPCPMTA) with SMTP id 8E.A9.25195.ED28A855; Wed, 24 Jun 2015 19:13:50 +0900 (KST) Date: Wed, 24 Jun 2015 10:13:50 +0000 (GMT) From: Ashish Sangwan Subject: Re: Re: Re: [PATCH] fsnotify: fix a crash due to invalid virtual address To: Jan Kara Cc: "linux-fsdevel@vger.kernel.org" , Andrew Morton , Eric Paris , AMIT SAHRAWAT , Namjae Jeon , PANKAJ MISHRA , Lino Sanfilippo Reply-to: a.sangwan@samsung.com MIME-version: 1.0 X-MTR: 20150624101205908@a.sangwan Msgkey: 20150624101205908@a.sangwan X-EPLocale: en_US.windows-1252 X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-MLAttribute: X-RootMTR: 20150624101205908@a.sangwan X-ParentMTR: X-ArchiveUser: X-CPGSPASS: N X-ConfirmMail: N,general Content-transfer-encoding: base64 Content-type: text/plain; charset=windows-1252 MIME-version: 1.0 Message-id: <166776534.176181435140830045.JavaMail.weblogic@ep2mlwas04a> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrOIsWRmVeSWpSXmKPExsWyRsSkVleiuSvU4GCrksWevSdZHBg9Pm+S C2CM4rJJSc3JLEst0rdL4MpY0L6XreCPRcX1n/cZGxj3mHcxcnIICahInF+xhBnElhAwkeiY sIQNwhaTuHBvPZDNBVSzlFFi8497LDBFCzb3MUEk5jBKHL1wmR0kwSKgKnGptQHMZhPQlXix ahcTiC0sECgx48gPsLiIgKzE/oPvWUGamQWOMEk8+jeRGeIMeYnbj9+CFfEKCEqcnPkEapuS xPY5zxgh4soSRxfvYoeIy0ksmXqZCcLmlZjR/pQFJj7t6xqod6Qlzs/awAjzzuLvj6Hi/BLH bu+A6hWQmHrmIFSNusTfa79ZIWw+iTUL37LA1O86tZwZZtf9LXOheiUktrY8AatnFlCUmNL9 kB3CNpA4smgOK7pfeAU8JB4de8QI8ryEwEQOiWfX57JNYFSahaRuFpJZs5DMQlazgJFlFaNo akFyQXFSepGJXnFibnFpXrpecn7uJkZgejj979mEHYz3DlgfYhTgYFTi4V1Q3xUqxJpYVlyZ e4jRFBhRE5mlRJPzgUkoryTe0NjMyMLUxNTYyNzSTEmc97XUz2AhgfTEktTs1NSC1KL4otKc 1OJDjEwcnFINjNr5FkcWzmkSn/PONvZ6+2r5KxunOThtLeSu3bzG9kR3SzrTEYWZnVnr9mmF zPfN3NUsob8g1/7+tum/3eZcF54auu7XtGXTrhy027g87RMLl5unx/Gnp391Bec+mvP5uJ2b Ug2b4kONjUKs9hdNElVmSoQub/611O/fmQOrlioIhl6YeCzTdYESS3FGoqEWc1FxIgBJKSgA CgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrBKsWRmVeSWpSXmKPExsVy+t/tPt17TV2hBoc/qVns2XuSxYHR4/Mm uQDGqDSbjNTElNQihdS85PyUzLx0WyXv4HjneFMzA0NdQ0sLcyWFvMTcVFslF58AXbfMHKCh SgpliTmlQKGAxOJiJX07m6L80pJUhYz84hJbpWhDcyM9IwM9UyM9Q9NYK0MDAyNToJqEtIwF 7XvZCv5YVFz/eZ+xgXGPeRcjJ4eQgIrE+RVLmEFsCQETiQWb+5ggbDGJC/fWs3UxcgHVzGGU OHrhMjtIgkVAVeJSawOYzSagK/Fi1S6wBmGBQIkZR36AxUUEZCX2H3zPCtLMLHCESeLRv4nM ENvkJW4/fgtWxCsgKHFy5hMWiG1KEtvnPGOEiCtLHF28ix0iLiexZOplqIt4JWa0P2WBiU/7 ugbqammJ87M2MMJcvfj7Y6g4v8Sx2zugegUkpp45CFWjLvH32m9WCJtPYs3Ctyww9btOLWeG 2XV/y1yoXgmJrS1PwOqZBRQlpnQ/ZIewDSSOLJrDiu4XXgEPiUfHHjFOYJSdhSQ1C0n7LCTt yGoWMLKsYhRNLUguKE5KrzDRK07MLS7NS9dLzs/dxAhORc+W7GBsuGB9iFGAg1GJh3dGRVeo EGtiWXFl7iFGCQ5mJRFenQagEG9KYmVValF+fFFpTmrxIUZTYLRNZJYSTc4Hpsm8knhDYxNz U2NTCwNDc3MzJXHe/+dyQ4QE0hNLUrNTUwtSi2D6mDg4pRoYb834/Sepdu8aaf/9W1IOKH19 WxHLe6l0neOhIOcjRwX+7rlQuPR7pcL8q9w1N7/2Ff/f7HlzBbu1aMQGljM/T01P+hwWtMdT /JIk54XXq76lHUz/Uu37bct36TnfLj9Nsvnilhop5HzL6154Q4vAcTH/B1dFr880KdjYrsHx f4dQZNLpG7/mmyqxFGckGmoxFxUnAgCK0GKmWwMAAA== DLP-Filter: Pass X-CFilter-Loop: Reflected Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Jan, > On Tue 23-06-15 12:05:51, Ashish Sangwan wrote: > > > Looking into this in more detail, it might be worthwhile to revisit how > > > mark_mutex is used since at least fanotify and dnotify use it for more than > > > just a protection of list of group marks and untangling this would simplify > > > things. But that's a longer term goal. > > > > > > A relatively simple fix for your issue is to split group list of marks into > > > a list of inode marks and a list of mount marks. Then destroying becomes > > > much simpler because we always discard the whole list (or both of them) and > > > we can easily avoid problems with list corruption when dropping the > > > mark_mutex. I can write the patch later or you can do that if you are > > Sorry I could not understand why the group's list of marks needs to be split. > > I was browsing through the old code, from the days mark_mutex was not present > > and it looked like below: > > void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group, > > unsigned int flags) > > { > > struct fsnotify_mark *lmark, *mark; > > LIST_HEAD(free_list); > > > > spin_lock(&group->mark_lock); > > list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) { > > if (mark->flags & flags) { > > list_add(&mark->free_g_list, &free_list); > > list_del_init(&mark->g_list); > > fsnotify_get_mark(mark); > > } > > } > > spin_unlock(&group->mark_lock); > > > > list_for_each_entry_safe(mark, lmark, &free_list, free_g_list) { > > fsnotify_destroy_mark(mark); > > fsnotify_put_mark(mark); > > } > > } > > How about using a temporary onstack list_head like above? > > So we can use a temporary list_head for entries selected for destruction as > well. *But* in either case you have to be careful because even the > temporary free_list can be modified by concurrent mark destruction e.g. from > fsnotify_free_marks_by_inode() call as each mark is in two lists - one > hanging from inode / mount and one hanging from the notification group. True. > > Actually, looking into it even fsnotify_destroy_marks() doesn't seem to be > properly protected from the race. Sigh. True. So anything which is calling fsnotify_destroy_mark is not protected against concurrent deletion from fsnotify_clear_marks_by_group_flags() and vice versa. Plus, as you rightly pointed, we have both the inode mark and vfsmount sharing the same list. So even if fsnotify_clear_marks_by_group_flags is for removing inode mark, a parallel fsnotify_destroy_mark for vfsmount can cause crash as they are sharing same list. Can you check below patch? It is untested, just want to know if the approach is correct or not. If it seems ok, I can send a tested patch later. diff --git a/fs/notify/mark.c b/fs/notify/mark.c index d90deaa..d83ec7d 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -124,14 +124,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark, spin_lock(&mark->lock); - /* something else already called this function on this mark */ - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) { - spin_unlock(&mark->lock); - return; - } - - mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; - if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) { inode = mark->i.inode; fsnotify_destroy_inode_mark(mark); @@ -188,7 +180,10 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark, struct fsnotify_group *group) { mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING); - fsnotify_destroy_mark_locked(mark, group); + if (mark->flags & FSNOTIFY_MARK_FLAG_ALIVE) { + mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; + fsnotify_destroy_mark_locked(mark, group); + } mutex_unlock(&group->mark_mutex); } @@ -293,14 +288,27 @@ void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group, unsigned int flags) { struct fsnotify_mark *lmark, *mark; - + LIST_HEAD(free_list); + mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING); - list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) { + /* Pass 1 : clear the alive flag and move to free list */ + list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) if (mark->flags & flags) { + /* + * If the mark is present on group's mark list + * it has to be alive. + */ + WARN_ON(!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)); + list_del_init(&mark->g_list); + list_add(&mark->g_list, &free_list); + mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; + } + + /* Pass 2: remove mark */ + list_for_each_entry_safe(mark, lmark, &free_list, g_list) { fsnotify_get_mark(mark); fsnotify_destroy_mark_locked(mark, group); fsnotify_put_mark(mark); - } } mutex_unlock(&group->mark_mutex); }