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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 29 Jun 2022 11:23:33 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Liu Song <liusong@...ux.alibaba.com>, axboe@...nel.dk
Cc:     linux-block@...r.kernel.org, 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.

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.