[PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[David Jeffery]

There is a race between changing I/O elevator and request_queue removal
which can trigger the warning in kobject_add_internal.  A program can
use sysfs to request a change of elevator at the same time another task
is unregistering the request_queue the elevator would be attached to.
The elevator's kobject will then attempt to be connected to the
request_queue in the object tree when the request_queue has just been
removed from sysfs.  This triggers the warning in kobject_add_internal
as the request_queue no longer has a sysfs directory:

kobject_add_internal failed for iosched (error: -2 parent: queue)
------------[ cut here ]------------
WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0


To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when
changing the elevator and use the request_queue's sysfs_lock to
serialize between clearing the flag and the elevator testing the flag.

Signed-off-by: David Jeffery <djeffery@redhat.com>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)


diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 27aceab..b8362c0 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return;
 
+	mutex_lock(&q->sysfs_lock);
 	queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
+	mutex_unlock(&q->sysfs_lock);
 
 	wbt_exit(q);
 
diff --git a/block/elevator.c b/block/elevator.c
index 4bb2f0c..51da592 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -1055,6 +1055,10 @@ 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;
+
 	/*
 	 * Special case for mq, turn off scheduling
 	 */

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[Ming Lei]

On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:
> There is a race between changing I/O elevator and request_queue removal
> which can trigger the warning in kobject_add_internal.  A program can
> use sysfs to request a change of elevator at the same time another task
> is unregistering the request_queue the elevator would be attached to.
> The elevator's kobject will then attempt to be connected to the
> request_queue in the object tree when the request_queue has just been
> removed from sysfs.  This triggers the warning in kobject_add_internal
> as the request_queue no longer has a sysfs directory:
>
> kobject_add_internal failed for iosched (error: -2 parent: queue)
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0
>
>
> To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when
> changing the elevator and use the request_queue's sysfs_lock to
> serialize between clearing the flag and the elevator testing the flag.

I remember I saw this issue too.

>
> Signed-off-by: David Jeffery <djeffery@redhat.com>
> ---
>  block/blk-sysfs.c |    2 ++
>  block/elevator.c  |    4 ++++
>  2 files changed, 6 insertions(+)
>
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 27aceab..b8362c0 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>         if (WARN_ON(!q))
>                 return;
>
> +       mutex_lock(&q->sysfs_lock);
>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
> +       mutex_unlock(&q->sysfs_lock);

Could you share why the lock of 'q->sysfs_lock' is needed here?

>
>         wbt_exit(q);
>
> diff --git a/block/elevator.c b/block/elevator.c
> index 4bb2f0c..51da592 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -1055,6 +1055,10 @@ 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;
> +

I suggest to check 'e->registered' here, which should be more
reasonable or straightforward.

-- 
Ming Lei

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[David Jeffery]

On 08/07/2017 07:53 PM, Ming Lei wrote:
> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:

>>
>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>> ---
>>  block/blk-sysfs.c |    2 ++
>>  block/elevator.c  |    4 ++++
>>  2 files changed, 6 insertions(+)
>>
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index 27aceab..b8362c0 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>>         if (WARN_ON(!q))
>>                 return;
>>
>> +       mutex_lock(&q->sysfs_lock);
>>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
>> +       mutex_unlock(&q->sysfs_lock);
> 
> Could you share why the lock of 'q->sysfs_lock' is needed here?

As the elevator change is initiated through a sysfs attr file in the
queue directory, the task doing the elevator change already acquires the
q->sysfs_lock before it can try and change the elevator.  Adding the
lock around clearing QUEUE_FLAG_REGISTERED ensures that the queue state
will be stable while the elevator is being changed.  It prevents a race
condition where the bit is checked but then cleared and queue removed
from sysfs before the elevator change completes.

> 
>>
>>         wbt_exit(q);
>>
>> diff --git a/block/elevator.c b/block/elevator.c
>> index 4bb2f0c..51da592 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -1055,6 +1055,10 @@ 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;
>> +
> 
> I suggest to check 'e->registered' here, which should be more
> reasonable or straightforward.
> 

e->registered is not the state needing to be checked.  We need to know
the state of the associated request queue.

Before changing the elevator, we need to ensure the request queue is
still connected to sysfs.  i.e. We need to know that kobject_del has not
been called on the request queue.  When QUEUE_FLAG_REGISTERED is not set
it means the request queue either has had kobject_del called or will
have it called soon, so we should fail the elevator change attempt.

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[Ming Lei]

Hi David,

On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery <djeffery@redhat.com> wrote:
> On 08/07/2017 07:53 PM, Ming Lei wrote:
>> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:
>
>>>
>>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>>> ---
>>>  block/blk-sysfs.c |    2 ++
>>>  block/elevator.c  |    4 ++++
>>>  2 files changed, 6 insertions(+)
>>>
>>>
>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>> index 27aceab..b8362c0 100644
>>> --- a/block/blk-sysfs.c
>>> +++ b/block/blk-sysfs.c
>>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>>>         if (WARN_ON(!q))
>>>                 return;
>>>
>>> +       mutex_lock(&q->sysfs_lock);
>>>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
>>> +       mutex_unlock(&q->sysfs_lock);
>>
>> Could you share why the lock of 'q->sysfs_lock' is needed here?
>
> As the elevator change is initiated through a sysfs attr file in the
> queue directory, the task doing the elevator change already acquires the
> q->sysfs_lock before it can try and change the elevator.  Adding the

It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock.

> lock around clearing QUEUE_FLAG_REGISTERED ensures that the queue state
> will be stable while the elevator is being changed.  It prevents a race
> condition where the bit is checked but then cleared and queue removed
> from sysfs before the elevator change completes.
>
>>
>>>
>>>         wbt_exit(q);
>>>
>>> diff --git a/block/elevator.c b/block/elevator.c
>>> index 4bb2f0c..51da592 100644
>>> --- a/block/elevator.c
>>> +++ b/block/elevator.c
>>> @@ -1055,6 +1055,10 @@ 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;
>>> +
>>
>> I suggest to check 'e->registered' here, which should be more
>> reasonable or straightforward.
>>
>
> e->registered is not the state needing to be checked.  We need to know
> the state of the associated request queue.
>
> Before changing the elevator, we need to ensure the request queue is
> still connected to sysfs.  i.e. We need to know that kobject_del has not
> been called on the request queue.  When QUEUE_FLAG_REGISTERED is not set
> it means the request queue either has had kobject_del called or will
> have it called soon, so we should fail the elevator change attempt.

elv_unregister_queue() is called in blk_unregister_queue() too, that is why
I suggest to check e->registered.


-- 
Ming Lei

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[Ming Lei]

On Wed, Aug 9, 2017 at 9:44 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi David,
>
> On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery <djeffery@redhat.com> wrote:
>> On 08/07/2017 07:53 PM, Ming Lei wrote:
>>> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:
>>
>>>>
>>>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>>>> ---
>>>>  block/blk-sysfs.c |    2 ++
>>>>  block/elevator.c  |    4 ++++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>>
>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>> index 27aceab..b8362c0 100644
>>>> --- a/block/blk-sysfs.c
>>>> +++ b/block/blk-sysfs.c
>>>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>>>>         if (WARN_ON(!q))
>>>>                 return;
>>>>
>>>> +       mutex_lock(&q->sysfs_lock);
>>>>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
>>>> +       mutex_unlock(&q->sysfs_lock);
>>>
>>> Could you share why the lock of 'q->sysfs_lock' is needed here?
>>
>> As the elevator change is initiated through a sysfs attr file in the
>> queue directory, the task doing the elevator change already acquires the
>> q->sysfs_lock before it can try and change the elevator.  Adding the
>
> It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock.

Looks I was wrong, and the store is from queue_attr_store() instead of
elv_attr_store().

I can reproduce the issue and this patch can fix the issue in my test
on scsi_debug,
so:

        Tested-by: Ming Lei <ming.lei@redhat.com>

And it is a typical race between removing queue kobj and adding children of
this queue kobj, we can't acquire q->sysfs_lock in blk_unregister_queue()
because of sysfs's limit(may cause deadlock), so one state has to be used
for this purpose, just like what register/unregister hctx kobjs does,
and it should
be fine to use QUEUE_FLAG_REGISTERED here. More changes are required if
we use e->registered, so this patch looks fine:

       Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming Lei

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[Ming Lei]

On Wed, Aug 23, 2017 at 11:34 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Wed, Aug 9, 2017 at 9:44 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>> Hi David,
>>
>> On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery <djeffery@redhat.com> wrote:
>>> On 08/07/2017 07:53 PM, Ming Lei wrote:
>>>> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:
>>>
>>>>>
>>>>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>>>>> ---
>>>>>  block/blk-sysfs.c |    2 ++
>>>>>  block/elevator.c  |    4 ++++
>>>>>  2 files changed, 6 insertions(+)
>>>>>
>>>>>
>>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>>> index 27aceab..b8362c0 100644
>>>>> --- a/block/blk-sysfs.c
>>>>> +++ b/block/blk-sysfs.c
>>>>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>>>>>         if (WARN_ON(!q))
>>>>>                 return;
>>>>>
>>>>> +       mutex_lock(&q->sysfs_lock);
>>>>>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
>>>>> +       mutex_unlock(&q->sysfs_lock);
>>>>
>>>> Could you share why the lock of 'q->sysfs_lock' is needed here?
>>>
>>> As the elevator change is initiated through a sysfs attr file in the
>>> queue directory, the task doing the elevator change already acquires the
>>> q->sysfs_lock before it can try and change the elevator.  Adding the
>>
>> It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock.
>
> Looks I was wrong, and the store is from queue_attr_store() instead of
> elv_attr_store().
>
> I can reproduce the issue and this patch can fix the issue in my test
> on scsi_debug,
> so:
>
>         Tested-by: Ming Lei <ming.lei@redhat.com>
>
> And it is a typical race between removing queue kobj and adding children of
> this queue kobj, we can't acquire q->sysfs_lock in blk_unregister_queue()
> because of sysfs's limit(may cause deadlock), so one state has to be used
> for this purpose, just like what register/unregister hctx kobjs does,
> and it should
> be fine to use QUEUE_FLAG_REGISTERED here. More changes are required if
> we use e->registered, so this patch looks fine:
>
>        Reviewed-by: Ming Lei <ming.lei@redhat.com>


Hi Jens,

Could you consider this patch for v4.13 or v4.14?


Thanks,
Ming Lei

Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed

[Jens Axboe]

On 08/27/2017 07:36 PM, Ming Lei wrote:
> On Wed, Aug 23, 2017 at 11:34 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Wed, Aug 9, 2017 at 9:44 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> Hi David,
>>>
>>> On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery <djeffery@redhat.com> wrote:
>>>> On 08/07/2017 07:53 PM, Ming Lei wrote:
>>>>> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:
>>>>
>>>>>>
>>>>>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>>>>>> ---
>>>>>>  block/blk-sysfs.c |    2 ++
>>>>>>  block/elevator.c  |    4 ++++
>>>>>>  2 files changed, 6 insertions(+)
>>>>>>
>>>>>>
>>>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>>>> index 27aceab..b8362c0 100644
>>>>>> --- a/block/blk-sysfs.c
>>>>>> +++ b/block/blk-sysfs.c
>>>>>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
>>>>>>         if (WARN_ON(!q))
>>>>>>                 return;
>>>>>>
>>>>>> +       mutex_lock(&q->sysfs_lock);
>>>>>>         queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
>>>>>> +       mutex_unlock(&q->sysfs_lock);
>>>>>
>>>>> Could you share why the lock of 'q->sysfs_lock' is needed here?
>>>>
>>>> As the elevator change is initiated through a sysfs attr file in the
>>>> queue directory, the task doing the elevator change already acquires the
>>>> q->sysfs_lock before it can try and change the elevator.  Adding the
>>>
>>> It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock.
>>
>> Looks I was wrong, and the store is from queue_attr_store() instead of
>> elv_attr_store().
>>
>> I can reproduce the issue and this patch can fix the issue in my test
>> on scsi_debug,
>> so:
>>
>>         Tested-by: Ming Lei <ming.lei@redhat.com>
>>
>> And it is a typical race between removing queue kobj and adding children of
>> this queue kobj, we can't acquire q->sysfs_lock in blk_unregister_queue()
>> because of sysfs's limit(may cause deadlock), so one state has to be used
>> for this purpose, just like what register/unregister hctx kobjs does,
>> and it should
>> be fine to use QUEUE_FLAG_REGISTERED here. More changes are required if
>> we use e->registered, so this patch looks fine:
>>
>>        Reviewed-by: Ming Lei <ming.lei@redhat.com>
> 
> 
> Hi Jens,
> 
> Could you consider this patch for v4.13 or v4.14?

Yep, added for 4.14, thanks.

-- 
Jens Axboe