From patchwork Wed Aug 24 09:55:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 9297349 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id CC944607D0 for ; Wed, 24 Aug 2016 10:02:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BAD8F28ECC for ; Wed, 24 Aug 2016 10:02:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id ACE5B28ED5; Wed, 24 Aug 2016 10:02:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1254328ECC for ; Wed, 24 Aug 2016 10:02:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932638AbcHXKCO (ORCPT ); Wed, 24 Aug 2016 06:02:14 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:38141 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932368AbcHXKCN (ORCPT ); Wed, 24 Aug 2016 06:02:13 -0400 Received: by mail-wm0-f51.google.com with SMTP id o80so19288815wme.1 for ; Wed, 24 Aug 2016 03:02:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=ZB6/o1IjpS5Z3dQy9suifGkz+5g74eGo2WndtqGQTDw=; b=Xy7UIoh2zuBZhAxFzKNa0b2lIlsBcy8S2vk5nFcAWNWaQESsYLlKw4NcbKpqS5HPeO ICbZqOi+ndD6hGUtENs8Ie8hWD0bRHL8COgEquvhKMi7nfBea8Wlk+9aUKPDvOmBXEgG SUdppQ3kuyytbUc+PgtYO6coRGS1fBGygOZuceK1Yg0Uo46GPkd702kvE173RC4FTiaF vzboHx9cCZC7Py4griji8FXFgUffQOu9+l4RMlZW+4ZkxodlAjyyI4MeypT6guKACBcl bzfVAMKPQllvl+x926sy1WSKNd05oH9ApLqO0K03Xcsb9UHax5n3AbQsVS9hj2ylWSTu KW8g== X-Gm-Message-State: AE9vXwNTZbj97FQr+kuaEvWzRrr16L0uq9c4a9u6/e1fERrD2wUM/nPTQBrp4l908TnPuwlj X-Received: by 10.28.13.143 with SMTP id 137mr2342383wmn.46.1472032541502; Wed, 24 Aug 2016 02:55:41 -0700 (PDT) Received: from veci.piliscsaba.szeredi.hu (pool-dsl-2c-0018.externet.hu. [217.173.44.24]) by smtp.gmail.com with ESMTPSA id kq2sm9070531wjc.41.2016.08.24.02.55.40 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 24 Aug 2016 02:55:40 -0700 (PDT) From: Miklos Szeredi To: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org, Al Viro , Jan Kara , Lino Sanfilippo , Eric Paris Subject: [PATCH] fanotify: fix race between fanotify_release() and fanotify_get_response() Date: Wed, 24 Aug 2016 11:55:39 +0200 Message-Id: <1472032539-30256-1-git-send-email-mszeredi@redhat.com> X-Mailer: git-send-email 2.5.5 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP List corruption was reported with a fanotify stress test. The bug turned out to be due to fsnotify_remove_event() being called on an event on the fanotify_data.access_list and protected by fanotify_data.access_lock instead of notification_mutex. This resulted in list_del_init() being run concurrently on the same list entry. This was introduced by commit 09e5f14e57c7 ("fanotify: on group destroy allow all waiters to bypass permission check") which made fanotify_get_response() flush out events when bypass_perm was set. The flush doesn't normally happen, since the wake_up() is called after the access_list was cleaned in fsnotify_release(). But the two are not synchronized, the fanotify_get_response() could still be processing a previous wakeup by the time bypass_perm is true. This was seen in the crashdumps in the report. This bug can be solved multiple ways, maybe the simplest is moving the bypass_perm setting after the list has been processed. In theory there's also a memory ordering problem here. atomic_inc() in itself doesn't imply a memory barrier, and spin_unlock() is a semi permeable barrier, so we need an explicit memory barrier so that the condition is precieved after the list is cleared. Similarly we need barriers for the case when event->response is set (i.e. non zero): fsnotify_destroy_event() might destroy the event while it's still on the access_list, since nothing guarantees that the storing the response value in event->response will be preceived after the list manipulation. So add the necessary barriers there as well. PS not sure why bypass_perm is an atomic_t, it could just as well be a boolean flag. PPS all this subtlety could be removed if the waitq was per-event, which would also allow better performance. Signed-off-by: Miklos Szeredi Fixes: 09e5f14e57c7 ("fanotify: on group destroy allow all waiters to bypass permission check") Cc: #v2.6.37+ Cc: Jan Kara Cc: Lino Sanfilippo Cc: Eric Paris --- fs/notify/fanotify/fanotify.c | 5 +++++ fs/notify/fanotify/fanotify_user.c | 25 +++++++++++++++++++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index d2f97ecca6a5..0d0cabd946e0 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -70,6 +70,11 @@ static int fanotify_get_response(struct fsnotify_group *group, wait_event(group->fanotify_data.access_waitq, event->response || atomic_read(&group->fanotify_data.bypass_perm)); + /* + * Pairs with smp_wmb() before storing event->response. This makes sure + * that the list_del_init() done on the event is preceived after this. + */ + smp_rmb(); if (!event->response) { /* bypass_perm set */ /* * Event was canceled because group is being destroyed. Remove diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 8e8e6bcd1d43..af57e75772a0 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -193,6 +193,10 @@ static int process_access_response(struct fsnotify_group *group, if (!event) return -ENOENT; + /* + * Make sure the dequeue is preceived before the store of "response" + */ + smp_wmb(); event->response = response; wake_up(&group->fanotify_data.access_waitq); @@ -305,6 +309,11 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, } else { #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS if (ret < 0) { + /* + * Make sure the dequeue is preceived before + * the store of "response" + */ + smp_wmb(); FANOTIFY_PE(kevent)->response = FAN_DENY; wake_up(&group->fanotify_data.access_waitq); break; @@ -365,26 +374,34 @@ static int fanotify_release(struct inode *ignored, struct file *file) * enter or leave access_list by now. */ spin_lock(&group->fanotify_data.access_lock); - - atomic_inc(&group->fanotify_data.bypass_perm); - list_for_each_entry_safe(event, next, &group->fanotify_data.access_list, fae.fse.list) { pr_debug("%s: found group=%p event=%p\n", __func__, group, event); list_del_init(&event->fae.fse.list); + /* + * Make sure the dequeue is preceived before the store of + * "response" + */ + smp_wmb(); event->response = FAN_ALLOW; } spin_unlock(&group->fanotify_data.access_lock); /* - * Since bypass_perm is set, newly queued events will not wait for + * After bypass_perm is set, newly queued events will not wait for * access response. Wake up the already sleeping ones now. + * + * Make sure we do this only *after* all events were taken off + * group->fanotify_data.access_list, otherwise the entry might be + * deleted concurrently by two entities, resulting in list corruption. + * * synchronize_srcu() in fsnotify_destroy_group() will wait for all * processes sleeping in fanotify_handle_event() waiting for access * response and thus also for all permission events to be freed. */ + atomic_inc(&group->fanotify_data.bypass_perm); wake_up(&group->fanotify_data.access_waitq); #endif