From patchwork Thu Aug 2 04:18:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 952570 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41gxl26n7mz9s2g; Thu, 2 Aug 2018 14:19:10 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1fl54r-0007Rn-5L; Thu, 02 Aug 2018 04:19:05 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.86_2) (envelope-from ) id 1fl54p-0007Q9-5c for kernel-team@lists.canonical.com; Thu, 02 Aug 2018 04:19:03 +0000 Received: from mail-pf1-f197.google.com ([209.85.210.197]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1fl54o-0001xC-Hn for kernel-team@lists.canonical.com; Thu, 02 Aug 2018 04:19:02 +0000 Received: by mail-pf1-f197.google.com with SMTP id v9-v6so660260pfn.6 for ; Wed, 01 Aug 2018 21:19:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=hGpTcfeWJQf8WzlXlKyV9eQmmQE3Hpf7eh227xf44wA=; b=nDcvMyZWv2gFFdf1WnAAifHeRs4hF2juyEEBlrBR/nTEqzSS06gBZZx80M2TuRZoti GR5oG8k1efk344aVmvDGaqyG9YCHY0YTGWDmHpdnhhk5UD0kBdcainVNLPzXWvGZf+1l 8TVGM9bEl2btBQt8EdDzC2gCrwJ+zBxZohKXbJ6VNg7stlzWO36aF/ClN1TJeKXKjRNn pW0gJHe5V4tj1YELZJyBk5yWWrRh4Q6tXcTQpUhFFv+hUeYbbk/xyk6XKeAcDWewukz9 gjZOvuLmMGmtrBs2u+AhKnfkOSODYTnpXuYUY0XJGxbpPYgOShShLw/GZAJYz/WoFwB0 le6A== X-Gm-Message-State: AOUpUlH8/8yZNPh4yLvQaGs21WZQwPJ75RBlfijMhusE0Ck2VSnD8sej YyLbAXMlAqffBv/p72h5Ly8uomgbGZBSsqCd5ir6V/bXpt8BHse9c0R9bk4AY40VbqwTj9wXJys 2iux0g0lsHZaDqQoWOFOzgLD6o5w/cZFrTfn2cWpv0HJem8q2 X-Received: by 2002:a17:902:bf06:: with SMTP id bi6-v6mr976239plb.76.1533183541216; Wed, 01 Aug 2018 21:19:01 -0700 (PDT) X-Google-Smtp-Source: AAOMgpe1lj2+roU+kz1ItYt1MEaZaLubuwBj6hQeWihO5BSJrK5Wkmx3C4fm+Le5WM3GeaeNeaFvNg== X-Received: by 2002:a17:902:bf06:: with SMTP id bi6-v6mr976232plb.76.1533183541040; Wed, 01 Aug 2018 21:19:01 -0700 (PDT) Received: from localhost.localdomain (124-171-193-200.dyn.iinet.net.au. [124.171.193.200]) by smtp.gmail.com with ESMTPSA id p73-v6sm952509pfk.186.2018.08.01.21.18.59 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 01 Aug 2018 21:19:00 -0700 (PDT) From: Daniel Axtens To: kernel-team@lists.canonical.com Subject: [SRU T][PATCH 3/6] cachefiles: Fix refcounting bug in backing-file read monitoring Date: Thu, 2 Aug 2018 14:18:47 +1000 Message-Id: <20180802041850.22961-4-daniel.axtens@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180802041850.22961-1-daniel.axtens@canonical.com> References: <20180802041850.22961-1-daniel.axtens@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Kiran Kumar Modukuri BugLink: https://bugs.launchpad.net/bugs/1774336 cachefiles_read_waiter() has the right to access a 'monitor' object by virtue of being called under the waitqueue lock for one of the pages in its purview. However, it has no ref on that monitor object or on the associated operation. What it is allowed to do is to move the monitor object to the operation's to_do list, but once it drops the work_lock, it's actually no longer permitted to access that object. However, it is trying to enqueue the retrieval operation for processing - but it can only do this via a pointer in the monitor object, something it shouldn't be doing. If it doesn't enqueue the operation, the operation may not get processed. If the order is flipped so that the enqueue is first, then it's possible for the work processor to look at the to_do list before the monitor is enqueued upon it. Fix this by getting a ref on the operation so that we can trust that it will still be there once we've added the monitor to the to_do list and dropped the work_lock. The op can then be enqueued after the lock is dropped. The bug can manifest in one of a couple of ways. The first manifestation looks like: FS-Cache: FS-Cache: Assertion failed FS-Cache: 6 == 5 is false ------------[ cut here ]------------ kernel BUG at fs/fscache/operation.c:494! RIP: 0010:fscache_put_operation+0x1e3/0x1f0 ... fscache_op_work_func+0x26/0x50 process_one_work+0x131/0x290 worker_thread+0x45/0x360 kthread+0xf8/0x130 ? create_worker+0x190/0x190 ? kthread_cancel_work_sync+0x10/0x10 ret_from_fork+0x1f/0x30 This is due to the operation being in the DEAD state (6) rather than INITIALISED, COMPLETE or CANCELLED (5) because it's already passed through fscache_put_operation(). The bug can also manifest like the following: kernel BUG at fs/fscache/operation.c:69! ... [exception RIP: fscache_enqueue_operation+246] ... #7 [ffff883fff083c10] fscache_enqueue_operation at ffffffffa0b793c6 #8 [ffff883fff083c28] cachefiles_read_waiter at ffffffffa0b15a48 #9 [ffff883fff083c48] __wake_up_common at ffffffff810af028 I'm not entirely certain as to which is line 69 in Lei's kernel, so I'm not entirely clear which assertion failed. Fixes: 9ae326a69004 ("CacheFiles: A cache that backs onto a mounted filesystem") Reported-by: Lei Xue Reported-by: Vegard Nossum Reported-by: Anthony DeRobertis Reported-by: NeilBrown Reported-by: Daniel Axtens Reported-by: Kiran Kumar Modukuri Signed-off-by: David Howells Reviewed-by: Daniel Axtens (cherry picked from commit 934140ab028713a61de8bca58c05332416d037d1) Signed-off-by: Daniel Axtens --- fs/cachefiles/rdwr.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c index a1210b0322e0..725a0e20a913 100644 --- a/fs/cachefiles/rdwr.c +++ b/fs/cachefiles/rdwr.c @@ -27,6 +27,7 @@ static int cachefiles_read_waiter(wait_queue_t *wait, unsigned mode, struct cachefiles_one_read *monitor = container_of(wait, struct cachefiles_one_read, monitor); struct cachefiles_object *object; + struct fscache_retrieval *op = monitor->op; struct wait_bit_key *key = _key; struct page *page = wait->private; @@ -51,16 +52,22 @@ static int cachefiles_read_waiter(wait_queue_t *wait, unsigned mode, list_del(&wait->task_list); /* move onto the action list and queue for FS-Cache thread pool */ - ASSERT(monitor->op); + ASSERT(op); - object = container_of(monitor->op->op.object, - struct cachefiles_object, fscache); + /* We need to temporarily bump the usage count as we don't own a ref + * here otherwise cachefiles_read_copier() may free the op between the + * monitor being enqueued on the op->to_do list and the op getting + * enqueued on the work queue. + */ + fscache_get_retrieval(op); + object = container_of(op->op.object, struct cachefiles_object, fscache); spin_lock(&object->work_lock); - list_add_tail(&monitor->op_link, &monitor->op->to_do); + list_add_tail(&monitor->op_link, &op->to_do); spin_unlock(&object->work_lock); - fscache_enqueue_retrieval(monitor->op); + fscache_enqueue_retrieval(op); + fscache_put_retrieval(op); return 0; }