From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:40474 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752725AbeDSUTb (ORCPT ); Thu, 19 Apr 2018 16:19:31 -0400 Date: Thu, 19 Apr 2018 22:19:29 +0200 From: Jan Kara To: Robert Kolchmeyer Cc: jack@suse.cz, amir73il@gmail.com, mszeredi@redhat.com, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v2] fsnotify: Fix fsnotify_mark_connector race Message-ID: <20180419201929.536vhhxpoblscppb@quack2.suse.cz> References: <20180419000525.21673-1-rkolchmeyer@google.com> <20180419174433.53058-1-rkolchmeyer@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180419174433.53058-1-rkolchmeyer@google.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 19-04-18 10:44:33, Robert Kolchmeyer wrote: > fsnotify() acquires a reference to a fsnotify_mark_connector through > the SRCU-protected pointer to_tell->i_fsnotify_marks. However, it > appears that no precautions are taken in fsnotify_put_mark() to > ensure that fsnotify() drops its reference to this > fsnotify_mark_connector before assigning a value to its 'destroy_next' > field. This can result in fsnotify_put_mark() assigning a value > to a connector's 'destroy_next' field right before fsnotify() tries to > traverse the linked list referenced by the connector's 'list' field. > Since these two fields are members of the same union, this behavior > results in a kernel panic. > > This issue is resolved by moving the connector's 'destroy_next' field > into the object pointer union. This should work since the object pointer > access is protected by both a spinlock and the value of the 'flags' > field, and the 'flags' field is cleared while holding the spinlock in > fsnotify_put_mark() before 'destroy_next' is updated. It shouldn't be > possible for another thread to accidentally read from the object pointer > after the 'destroy_next' field is updated. > > The offending behavior here is extremely unlikely; since > fsnotify_put_mark() removes references to a connector (specifically, > it ensures that the connector is unreachable from the inode it was > formerly attached to) before updating its 'destroy_next' field, a > sizeable chunk of code in fsnotify_put_mark() has to execute in the > short window between when fsnotify() acquires the connector reference > and saves the value of its 'list' field. On the HEAD kernel, I've only > been able to reproduce this by inserting a udelay(1) in fsnotify(). > However, I've been able to reproduce this issue without inserting a > udelay(1) anywhere on older unmodified release kernels, so I believe > it's worth fixing at HEAD. > > Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437 > > Fixes: 08991e83b7286635167bab40927665a90fb00d81 > Signed-off-by: Robert Kolchmeyer Thanks! I've added the patch to my tree and will push it to Linus for -rc3. Honza > --- > Changelog since v1: > - Solve problem by moving 'destroy_next' field into a different union instead > of calling synchronize_srcu(). > > include/linux/fsnotify_backend.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index 9f1edb92c97e..a3d13d874fd1 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -217,12 +217,10 @@ struct fsnotify_mark_connector { > union { /* Object pointer [lock] */ > struct inode *inode; > struct vfsmount *mnt; > - }; > - union { > - struct hlist_head list; > /* Used listing heads to free after srcu period expires */ > struct fsnotify_mark_connector *destroy_next; > }; > + struct hlist_head list; > }; > > /* > -- > 2.17.0.484.g0c8726318c-goog > -- Jan Kara SUSE Labs, CR