linux-kernel - Re: [PATCH] blk-mq: set BLK_MQ_S_STOPPED first to avoid unexpected queue work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aa888597-e905-40a6-89da-5341e2af44b3.liusong@linux.alibaba.com>
Date:   Thu, 30 Jun 2022 09:07:32 +0800
From:   "liusong" <liusong@...ux.alibaba.com>
To:     "axboe" <axboe@...nel.dk>, "Bart Van Assche" <bvanassche@....org>
Cc:     "linux-block" <linux-block@...r.kernel.org>,
        "linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] blk-mq: set BLK_MQ_S_STOPPED first to avoid unexpected queue work

>On 6/28/22 22:18, Liu Song wrote:
>> From: Liu Song <liusong@...ux.alibaba.com>
>>
>> In "__blk_mq_delay_run_hw_queue", BLK_MQ_S_STOPPED is checked first,
>> and then queue work, but in "blk_mq_stop_hw_queue", execute cancel
>> work first and then set BLK_MQ_S_STOPPED, so there is a risk of
>> queue work after setting BLK_MQ_S_STOPPED, which can be solved by
>> adjusting the order.
>>
>> Signed-off-by: Liu Song <liusong@...ux.alibaba.com>
>> ---
>>   block/blk-mq.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 93d9d60..865915e 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2258,9 +2258,9 @@ bool blk_mq_queue_stopped(struct request_queue *q)
>>    */
>>   void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
>>   {
>> - cancel_delayed_work(&hctx->run_work);
>> -
>>    set_bit(BLK_MQ_S_STOPPED, &hctx->state);
>> +
>> + cancel_delayed_work(&hctx->run_work);
>>   }
>>   EXPORT_SYMBOL(blk_mq_stop_hw_queue);
>
>What made you come up with this patch? Source code reading or something
>else? Please mention this in the patch description.

Hi,

I found this by source code reading.
It is true that "blk_mq_stop_hw_queue" does not guarantee any dispatch will be blocked,
but I think "blk_mq_stop_hw_queue" and "__blk_mq_delay_run_hw_queue" have a reverse
order in the processing logic of "BLK_MQ_S_STOPPED".
Part of the race problem can be solved only by adjusting the judgment order, so it is still valuable.

Thanks

>
>Regarding the above patch, I don't think this patch fixes the existing
>race between blk_mq_stop_hw_queue() and __blk_mq_delay_run_hw_queue(),
>not even if cancel_delayed_work_sync() would be used.
>
>The comment block above blk_mq_stop_hw_queue() clearly mentions that it
>is not guaranteed that this function stops dispatching of requests
>immediately. So why bother about fixing the existing race conditions that
>do not affect what is guaranteed by blk_mq_stop_hw_queue()?
>
>Thanks,
>
>Bart.