lkml.org 
[lkml]   [2020]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.9 29/90] blktrace: Fix potential deadlock between delete & sysfs ops
    Date
    From: Waiman Long <longman@redhat.com>

    commit 5acb3cc2c2e9d3020a4fee43763c6463767f1572 upstream.

    The lockdep code had reported the following unsafe locking scenario:

    CPU0 CPU1
    ---- ----
    lock(s_active#228);
    lock(&bdev->bd_mutex/1);
    lock(s_active#228);
    lock(&bdev->bd_mutex);

    *** DEADLOCK ***

    The deadlock may happen when one task (CPU1) is trying to delete a
    partition in a block device and another task (CPU0) is accessing
    tracing sysfs file (e.g. /sys/block/dm-1/trace/act_mask) in that
    partition.

    The s_active isn't an actual lock. It is a reference count (kn->count)
    on the sysfs (kernfs) file. Removal of a sysfs file, however, require
    a wait until all the references are gone. The reference count is
    treated like a rwsem using lockdep instrumentation code.

    The fact that a thread is in the sysfs callback method or in the
    ioctl call means there is a reference to the opended sysfs or device
    file. That should prevent the underlying block structure from being
    removed.

    Instead of using bd_mutex in the block_device structure, a new
    blk_trace_mutex is now added to the request_queue structure to protect
    access to the blk_trace structure.

    Suggested-by: Christoph Hellwig <hch@infradead.org>
    Signed-off-by: Waiman Long <longman@redhat.com>
    Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

    Fix typo in patch subject line, and prune a comment detailing how
    the code used to work.

    Signed-off-by: Jens Axboe <axboe@kernel.dk>
    Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    block/blk-core.c | 3 +++
    include/linux/blkdev.h | 1 +
    kernel/trace/blktrace.c | 18 ++++++++++++------
    3 files changed, 16 insertions(+), 6 deletions(-)

    diff --git a/block/blk-core.c b/block/blk-core.c
    index bdb906bbfe198..4987f312a95f4 100644
    --- a/block/blk-core.c
    +++ b/block/blk-core.c
    @@ -729,6 +729,9 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)

    kobject_init(&q->kobj, &blk_queue_ktype);

    +#ifdef CONFIG_BLK_DEV_IO_TRACE
    + mutex_init(&q->blk_trace_mutex);
    +#endif
    mutex_init(&q->sysfs_lock);
    spin_lock_init(&q->__queue_lock);

    diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
    index 2fc4ba6fa07f9..a8dfbad42d1b0 100644
    --- a/include/linux/blkdev.h
    +++ b/include/linux/blkdev.h
    @@ -446,6 +446,7 @@ struct request_queue {
    int node;
    #ifdef CONFIG_BLK_DEV_IO_TRACE
    struct blk_trace *blk_trace;
    + struct mutex blk_trace_mutex;
    #endif
    /*
    * for flush operations
    diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
    index bfa8bb3a6e196..ff1384c5884c5 100644
    --- a/kernel/trace/blktrace.c
    +++ b/kernel/trace/blktrace.c
    @@ -644,6 +644,12 @@ int blk_trace_startstop(struct request_queue *q, int start)
    }
    EXPORT_SYMBOL_GPL(blk_trace_startstop);

    +/*
    + * When reading or writing the blktrace sysfs files, the references to the
    + * opened sysfs or device files should prevent the underlying block device
    + * from being removed. So no further delete protection is really needed.
    + */
    +
    /**
    * blk_trace_ioctl: - handle the ioctls associated with tracing
    * @bdev: the block device
    @@ -661,7 +667,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
    if (!q)
    return -ENXIO;

    - mutex_lock(&bdev->bd_mutex);
    + mutex_lock(&q->blk_trace_mutex);

    switch (cmd) {
    case BLKTRACESETUP:
    @@ -687,7 +693,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
    break;
    }

    - mutex_unlock(&bdev->bd_mutex);
    + mutex_unlock(&q->blk_trace_mutex);
    return ret;
    }

    @@ -1656,7 +1662,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
    if (q == NULL)
    goto out_bdput;

    - mutex_lock(&bdev->bd_mutex);
    + mutex_lock(&q->blk_trace_mutex);

    if (attr == &dev_attr_enable) {
    ret = sprintf(buf, "%u\n", !!q->blk_trace);
    @@ -1675,7 +1681,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
    ret = sprintf(buf, "%llu\n", q->blk_trace->end_lba);

    out_unlock_bdev:
    - mutex_unlock(&bdev->bd_mutex);
    + mutex_unlock(&q->blk_trace_mutex);
    out_bdput:
    bdput(bdev);
    out:
    @@ -1717,7 +1723,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
    if (q == NULL)
    goto out_bdput;

    - mutex_lock(&bdev->bd_mutex);
    + mutex_lock(&q->blk_trace_mutex);

    if (attr == &dev_attr_enable) {
    if (!!value == !!q->blk_trace) {
    @@ -1747,7 +1753,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
    }

    out_unlock_bdev:
    - mutex_unlock(&bdev->bd_mutex);
    + mutex_unlock(&q->blk_trace_mutex);
    out_bdput:
    bdput(bdev);
    out:
    --
    2.20.1


    \
     
     \ /
      Last update: 2020-05-18 19:44    [W:8.631 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site