Re: [PATCH v3] scsi: ufs: core: Keep internal commands dispatchable during error handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent use-after-free of `pseudo_sdev` and its `request_queue` during driver removal.
--

commit 8a56a1b6be4e0430f65c83bfe4de68c1b55a6211
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Keep internal commands dispatchable during error handling

This commit unquiesces the pseudo SCSI device's request queue during error
handling so internal commands can be dispatched while logical units remain
quiesced. It balances this by re-quiescing the pseudo device queue in the
unprepare phase.

Link: https://lore.kernel.org/all/6f78c4bd-a70b-402d-abfd-599091b67674@acm.org/

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 234e18b5078f2..eb86db492f225 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6841,11 +6841,17 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>  	}
>  	/* Wait for ongoing ufshcd_queuecommand() calls to finish. */
>  	blk_mq_quiesce_tagset(&hba->host->tag_set);
> +	/*
> +	 * Internal commands are submitted on the pseudo SCSI device. Let them
> +	 * through so that the error handler can recover the link.
> +	 */
> +	blk_mq_unquiesce_queue(hba->host->pseudo_sdev->request_queue);

[Severity: High]
Is it possible for hba->host->pseudo_sdev and its request_queue to be freed
before we access it here?

When the driver is unbound, ufshcd_remove() calls scsi_remove_host() before
the error handler workqueue is drained and destroyed in ufshcd_hba_exit():

drivers/ufs/core/ufshcd.c:ufshcd_remove() {
        ...
        if (hba->scsi_host_added)
                scsi_remove_host(hba->host);
        ...
}

Calling scsi_remove_host() removes all attached SCSI devices, including the
pseudo_sdev, which drops its final reference and frees it.

If ufshcd_err_handler() executes concurrently, it checks
ufshcd_err_handling_should_stop() and drops the host_lock:

drivers/ufs/core/ufshcd.c:ufshcd_err_handler() {
        ...
        if (ufshcd_err_handling_should_stop(hba)) {
                // ...
        }
        spin_unlock_irqrestore(&hba->host->host_lock, flags);

        ufshcd_err_handling_prepare(hba);
        ...
}

Since ufshcd_err_handling_should_stop() might return false before teardown
flags like is_powered are set, could this race lead to a use-after-free
when we dereference hba->host->pseudo_sdev->request_queue?

>  	cancel_work_sync(&hba->eeh_work);
>  }
>  
>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  {
> +	blk_mq_quiesce_queue_nowait(hba->host->pseudo_sdev->request_queue);

[Severity: High]
Would the same use-after-free race occur here during the unprepare phase?

>  	blk_mq_unquiesce_tagset(&hba->host->tag_set);
>  	ufshcd_release(hba);
>  	if (ufshcd_is_clkscaling_supported(hba))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912131625.2301486-1-stanleyjhu@google.com?part=1