lkml.org 
[lkml]   [2018]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 289/366] cachefiles: Fix refcounting bug in backing-file read monitoring
    3.16.61-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Kiran Kumar Modukuri <kiran.modukuri@gmail.com>

    commit 934140ab028713a61de8bca58c05332416d037d1 upstream.

    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 <carmark.dlut@gmail.com>
    Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
    Reported-by: Anthony DeRobertis <aderobertis@metrics.net>
    Reported-by: NeilBrown <neilb@suse.com>
    Reported-by: Daniel Axtens <dja@axtens.net>
    Reported-by: Kiran Kumar Modukuri <kiran.modukuri@gmail.com>
    Signed-off-by: David Howells <dhowells@redhat.com>
    Reviewed-by: Daniel Axtens <dja@axtens.net>
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    fs/cachefiles/rdwr.c | 17 ++++++++++++-----
    1 file changed, 12 insertions(+), 5 deletions(-)

    --- a/fs/cachefiles/rdwr.c
    +++ b/fs/cachefiles/rdwr.c
    @@ -27,6 +27,7 @@ static int cachefiles_read_waiter(wait_q
    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_q
    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;
    }

    \
     
     \ /
      Last update: 2018-11-11 21:26    [W:4.107 / U:0.168 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site