Re: [PATCH 3/3] block: Protect less code with sysfs_lock in blk_{un,}register_queue()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 17, 2018 at 8:03 AM, Bart Van Assche <Bart.VanAssche@xxxxxxx> wrote:
> On Tue, 2018-01-16 at 17:32 -0500, Mike Snitzer wrote:
>> Therefore it seems to me that all queue_attr_{show,store} are racey vs
>> blk_unregister_queue() removing the 'queue' kobject.
>>
>> And it was just that __elevator_change() was myopicly fixed to address
>> the race whereas a more generic solution was/is needed.  But short of
>> that more generic fix your change will reintroduce the potential for
>> hitting the issue that commit e9a823fb34a8b fixed.
>>
>> In that light, think it best to leave blk_unregister_queue()'s
>> mutex_lock() above the QUEUE_FLAG_REGISTERED clearing _and_ update
>> queue_attr_{show,store} to test for QUEUE_FLAG_REGISTERED while holding
>> sysfs_lock.
>>
>> Then remove the unicorn test_bit for QUEUE_FLAG_REGISTERED from
>> __elevator_change().
>
> Thanks Mike for the feedback. However, I think a simpler approach exists than
> what has been described above, namely by unregistering the sysfs attributes
> earlier. How about the patch below?
>
> [PATCH] block: Protect less code with sysfs_lock in blk_{un,}register_queue()
> ---
>  block/blk-sysfs.c | 39 ++++++++++++++++++++++++++-------------
>  block/elevator.c  |  4 ----
>  2 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 4a6a40ffd78e..ce32366c6db7 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -853,6 +853,10 @@ struct kobj_type blk_queue_ktype = {
>         .release        = blk_release_queue,
>  };
>
> +/**
> + * blk_register_queue - register a block layer queue with sysfs
> + * @disk: Disk of which the request queue should be registered with sysfs.
> + */
>  int blk_register_queue(struct gendisk *disk)
>  {
>         int ret;
> @@ -909,11 +913,12 @@ int blk_register_queue(struct gendisk *disk)
>         if (q->request_fn || (q->mq_ops && q->elevator)) {
>                 ret = elv_register_queue(q);
>                 if (ret) {
> +                       mutex_unlock(&q->sysfs_lock);
>                         kobject_uevent(&q->kobj, KOBJ_REMOVE);
>                         kobject_del(&q->kobj);
>                         blk_trace_remove_sysfs(dev);
>                         kobject_put(&dev->kobj);
> -                       goto unlock;
> +                       return ret;
>                 }
>         }
>         ret = 0;
> @@ -923,6 +928,13 @@ int blk_register_queue(struct gendisk *disk)
>  }
>  EXPORT_SYMBOL_GPL(blk_register_queue);
>
> +/**
> + * blk_unregister_queue - counterpart of blk_register_queue()
> + * @disk: Disk of which the request queue should be unregistered from sysfs.
> + *
> + * Note: the caller is responsible for guaranteeing that this function is called
> + * after blk_register_queue() has finished.
> + */
>  void blk_unregister_queue(struct gendisk *disk)
>  {
>         struct request_queue *q = disk->queue;
> @@ -934,28 +946,29 @@ void blk_unregister_queue(struct gendisk *disk)
>         if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
>                 return;
>
> -       /*
> -        * Protect against the 'queue' kobj being accessed
> -        * while/after it is removed.
> -        */
> -       mutex_lock(&q->sysfs_lock);
> -
>         spin_lock_irq(q->queue_lock);
>         queue_flag_clear(QUEUE_FLAG_REGISTERED, q);
>         spin_unlock_irq(q->queue_lock);
>
> -       wbt_exit(q);
> -
> +       /*
> +        * Remove the sysfs attributes before unregistering the queue data
> +        * structures that can be modified through sysfs.
> +        */
> +       mutex_lock(&q->sysfs_lock);
>         if (q->mq_ops)
>                 blk_mq_unregister_dev(disk_to_dev(disk), q);
> -
> -       if (q->request_fn || (q->mq_ops && q->elevator))
> -               elv_unregister_queue(q);
> +       mutex_unlock(&q->sysfs_lock);
>
>         kobject_uevent(&q->kobj, KOBJ_REMOVE);
>         kobject_del(&q->kobj);

elevator switch still can come just after the above line code is completed,
so the previous warning addressed in e9a823fb34a8b can be triggered
again.

>         blk_trace_remove_sysfs(disk_to_dev(disk));
> -       kobject_put(&disk_to_dev(disk)->kobj);
>
> +       wbt_exit(q);
> +
> +       mutex_lock(&q->sysfs_lock);
> +       if (q->request_fn || (q->mq_ops && q->elevator))
> +               elv_unregister_queue(q);
>         mutex_unlock(&q->sysfs_lock);
> +
> +       kobject_put(&disk_to_dev(disk)->kobj);
>  }
> diff --git a/block/elevator.c b/block/elevator.c
> index e87e9b43aba0..4b7957b28a99 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -1080,10 +1080,6 @@ static int __elevator_change(struct request_queue *q, const char *name)
>         char elevator_name[ELV_NAME_MAX];
>         struct elevator_type *e;
>
> -       /* Make sure queue is not in the middle of being removed */
> -       if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
> -               return -ENOENT;
> -

The above check shouldn't be removed, as I explained above.



-- 
Ming Lei



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux