From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f195.google.com ([209.85.128.195]:35811 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753467AbeDSQRF (ORCPT ); Thu, 19 Apr 2018 12:17:05 -0400 Received: by mail-wr0-f195.google.com with SMTP id w3-v6so15592383wrg.2 for ; Thu, 19 Apr 2018 09:17:04 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180419092321.bdejnxjjotopkphd@quack2.suse.cz> References: <20180419000525.21673-1-rkolchmeyer@google.com> <20180419092321.bdejnxjjotopkphd@quack2.suse.cz> From: Robert Kolchmeyer Date: Thu, 19 Apr 2018 09:17:02 -0700 Message-ID: Subject: Re: [PATCH] fsnotify: Fix fsnotify_mark_connector race To: Jan Kara Cc: amir73il@gmail.com, mszeredi@redhat.com, linux-fsdevel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Sounds great. I'll send out that patch shortly. Thanks! On Thu, Apr 19, 2018 at 2:23 AM, Jan Kara wrote: > On Wed 18-04-18 17:05:25, 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. > > Ahh, I was reading this code just two days ago trying to find some issues > (due to reports of softlockups in fsnotify()) but this didn't occur to me. > Great spotting! > >> This issue is resolved by calling synchronize_srcu() before updating >> the connector's 'destroy_next' field in fsnotify_put_mark(). Since the >> relevant section of fsnotify() occurs in a SRCU read-side critical >> section, this will force fsnotify_put_mark() to wait for fsnotify() >> to finish operating on the connector before updating its 'destroy_next' >> field. Since fsnotify_put_mark() removes references to the connector >> before assigning its 'destroy_next' field, it shouldn't be possible for >> another thread to acquire a reference to the connector while >> fsnotify_put_mark() waits for fsnotify() to finish its work. >> >> 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. > > Oh, the bug definitely needs fixing. I somewhat dislike synchronize_srcu() > when destroying the connector though. The whole point of > connector_destroy_list is to batch synchronize_srcu() calls and move them > out of the relative fast path. So what I'd do instead is to move > 'destroy_next' from a union with 'list' to a union with 'inode' and 'mnt'. > Those are not safe to access under srcu anyway - you have to hold > conn->lock to get them and use conn->flags to determine the type under the > same lock. So in that union we are actually safe to use the space for > anything once conn->flags are cleared by > fsnotify_detach_connector_from_object(). Will you send a patch for that so > that you get a proper credit for all the debugging you did? :) Thanks! > > Honza > >> Fixes bug 199437: https://bugzilla.kernel.org/show_bug.cgi?id=199437 >> >> Fixes: 08991e83b7286635167bab40927665a90fb00d81 >> Signed-off-by: Robert Kolchmeyer >> --- >> fs/notify/mark.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/notify/mark.c b/fs/notify/mark.c >> index e9191b416434..358fc7de1e86 100644 >> --- a/fs/notify/mark.c >> +++ b/fs/notify/mark.c >> @@ -227,6 +227,7 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) >> iput(inode); >> >> if (free_conn) { >> + synchronize_srcu(&fsnotify_mark_srcu); >> spin_lock(&destroy_lock); >> conn->destroy_next = connector_destroy_list; >> connector_destroy_list = conn; >> -- >> 2.17.0.484.g0c8726318c-goog >> > -- > Jan Kara > SUSE Labs, CR