Re: [PATCH] block: Fix a race in the runtime power management code

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Bart,

On Sun, 2020-08-23 at 20:06 -0700, Bart Van Assche wrote:
> With the current implementation the following race can happen:
> * blk_pre_runtime_suspend() calls blk_freeze_queue_start() and
>   blk_mq_unfreeze_queue().
> * blk_queue_enter() calls blk_queue_pm_only() and that function returns
>   true.
> * blk_queue_enter() calls blk_pm_request_resume() and that function does
>   not call pm_request_resume() because the queue runtime status is
>   RPM_ACTIVE.
> * blk_pre_runtime_suspend() changes the queue status into RPM_SUSPENDING.
> 
> Fix this race by changing the queue runtime status into RPM_SUSPENDING
> before switching q_usage_counter to atomic mode.
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: stable <stable@vger.kernel.org>
> Fixes: 986d413b7c15 ("blk-mq: Enable support for runtime power management")
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/blk-pm.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-pm.c b/block/blk-pm.c
> index b85234d758f7..17bd020268d4 100644
> --- a/block/blk-pm.c
> +++ b/block/blk-pm.c
> @@ -67,6 +67,10 @@ int blk_pre_runtime_suspend(struct request_queue *q)
>  
>  	WARN_ON_ONCE(q->rpm_status != RPM_ACTIVE);
>  
> +	spin_lock_irq(&q->queue_lock);
> +	q->rpm_status = RPM_SUSPENDING;
> +	spin_unlock_irq(&q->queue_lock);
> +

Has below alternative way been considered that RPM_SUSPENDING is set
after blk_mq_unfreeze_queue()?

	blk_freeze_queue_start(q);

+	spin_lock_irq(&q->queue_lock);
+	q->rpm_status = RPM_SUSPENDING;
+	spin_unlock_irq(&q->queue_lock);


Otherwise requests can enter queue while rpm_status is RPM_SUSPENDING
during a small window, i.e., before blk_set_pm_only() is invoked. This
would make the definition of rpm_status ambiguous.

In this way, the racing could be also solved:

- Before blk_freeze_queue_start(), any requests shall be allowed to
enter queue
- blk_freeze_queue_start() freezes the queue and blocks all upcoming
requests (make them wait_event(q->mq_freeze_wq))
- rpm_status is set as RPM_SUSPENDING
- blk_mq_unfreeze_queue() wakes up q->mq_freeze_wq and then
blk_pm_request_resume() can be executed

Thanks,

Stanley Chu


>  	/*
>  	 * Increase the pm_only counter before checking whether any
>  	 * non-PM blk_queue_enter() calls are in progress to avoid that any
> @@ -89,15 +93,14 @@ int blk_pre_runtime_suspend(struct request_queue *q)
>  	/* Switch q_usage_counter back to per-cpu mode. */
>  	blk_mq_unfreeze_queue(q);
>  
> -	spin_lock_irq(&q->queue_lock);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		spin_lock_irq(&q->queue_lock);
> +		q->rpm_status = RPM_ACTIVE;
>  		pm_runtime_mark_last_busy(q->dev);
> -	else
> -		q->rpm_status = RPM_SUSPENDING;
> -	spin_unlock_irq(&q->queue_lock);
> +		spin_unlock_irq(&q->queue_lock);
>  
> -	if (ret)
>  		blk_clear_pm_only(q);
> +	}
>  
>  	return ret;
>  }