Re: [PATCH] scsi: core: Fix missing lock when read async_scan in Scsi_Host

[Damien Le Moal]

On 3/2/26 21:13, Chaohai Chen wrote:
> When setting the async_scan flag in host, the host lock was locked,
> but it is not locked during reading. Encapsulate the corresponding
> API to fix this issue.
> 
> Signed-off-by: Chaohai Chen <wdhh6@xxxxxxxxxx>
> ---
>  drivers/scsi/scsi_scan.c | 60 +++++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 60c06fa4ec32..8b63130ef2e5 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -122,6 +122,42 @@ struct async_scan_data {
>  	struct completion prev_finished;
>  };
>  
> +static bool scsi_test_async_scan(struct Scsi_Host *shost)
> +{
> +	bool async;
> +	unsigned long flags;
> +
> +	lockdep_assert_not_held(shost->host_lock);
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	async = shost->async_scan;
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +	return async;
> +}

Use an atomic ?

> +
> +static void scsi_set_async_scan(struct Scsi_Host *shost)
> +{
> +	unsigned long flags;
> +
> +	lockdep_assert_not_held(shost->host_lock);
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	shost->async_scan = 1;
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +}
> +
> +static void scsi_clear_async_scan(struct Scsi_Host *shost)
> +{
> +	unsigned long flags;
> +
> +	lockdep_assert_not_held(shost->host_lock);
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	shost->async_scan = 0;
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +}
> +
>  /*
>   * scsi_enable_async_suspend - Enable async suspend and resume
>   */
> @@ -1298,7 +1334,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  		goto out_free_result;
>  	}
>  
> -	res = scsi_add_lun(sdev, result, &bflags, shost->async_scan);
> +	res = scsi_add_lun(sdev, result, &bflags, scsi_test_async_scan(shost));
>  	if (res == SCSI_SCAN_LUN_PRESENT) {
>  		if (bflags & BLIST_KEY) {
>  			sdev->lockable = 0;
> @@ -1629,7 +1665,7 @@ struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
>  	scsi_autopm_get_target(starget);
>  
>  	mutex_lock(&shost->scan_mutex);
> -	if (!shost->async_scan)
> +	if (!scsi_test_async_scan(shost))
>  		scsi_complete_async_scans();
>  
>  	if (scsi_host_scan_allowed(shost) && scsi_autopm_get_host(shost) == 0) {
> @@ -1839,7 +1875,7 @@ void scsi_scan_target(struct device *parent, unsigned int channel,
>  		return;
>  
>  	mutex_lock(&shost->scan_mutex);
> -	if (!shost->async_scan)
> +	if (!scsi_test_async_scan(shost))
>  		scsi_complete_async_scans();
>  
>  	if (scsi_host_scan_allowed(shost) && scsi_autopm_get_host(shost) == 0) {
> @@ -1896,7 +1932,7 @@ int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
>  		return -EINVAL;
>  
>  	mutex_lock(&shost->scan_mutex);
> -	if (!shost->async_scan)
> +	if (!scsi_test_async_scan(shost))
>  		scsi_complete_async_scans();
>  
>  	if (scsi_host_scan_allowed(shost) && scsi_autopm_get_host(shost) == 0) {
> @@ -1943,13 +1979,12 @@ static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
>  static struct async_scan_data *scsi_prep_async_scan(struct Scsi_Host *shost)
>  {
>  	struct async_scan_data *data = NULL;
> -	unsigned long flags;
>  
>  	if (strncmp(scsi_scan_type, "sync", 4) == 0)
>  		return NULL;
>  
>  	mutex_lock(&shost->scan_mutex);
> -	if (shost->async_scan) {
> +	if (scsi_test_async_scan(shost)) {
>  		shost_printk(KERN_DEBUG, shost, "%s called twice\n", __func__);
>  		goto err;
>  	}
> @@ -1961,10 +1996,7 @@ static struct async_scan_data *scsi_prep_async_scan(struct Scsi_Host *shost)
>  	if (!data->shost)
>  		goto err;
>  	init_completion(&data->prev_finished);
> -
> -	spin_lock_irqsave(shost->host_lock, flags);
> -	shost->async_scan = 1;
> -	spin_unlock_irqrestore(shost->host_lock, flags);
> +	scsi_set_async_scan(shost);
>  	mutex_unlock(&shost->scan_mutex);
>  
>  	spin_lock(&async_scan_lock);
> @@ -1992,7 +2024,6 @@ static struct async_scan_data *scsi_prep_async_scan(struct Scsi_Host *shost)
>  static void scsi_finish_async_scan(struct async_scan_data *data)
>  {
>  	struct Scsi_Host *shost;
> -	unsigned long flags;
>  
>  	if (!data)
>  		return;
> @@ -2001,7 +2032,7 @@ static void scsi_finish_async_scan(struct async_scan_data *data)
>  
>  	mutex_lock(&shost->scan_mutex);
>  
> -	if (!shost->async_scan) {
> +	if (!scsi_test_async_scan(shost)) {
>  		shost_printk(KERN_INFO, shost, "%s called twice\n", __func__);
>  		dump_stack();
>  		mutex_unlock(&shost->scan_mutex);
> @@ -2011,10 +2042,7 @@ static void scsi_finish_async_scan(struct async_scan_data *data)
>  	wait_for_completion(&data->prev_finished);
>  
>  	scsi_sysfs_add_devices(shost);
> -
> -	spin_lock_irqsave(shost->host_lock, flags);
> -	shost->async_scan = 0;
> -	spin_unlock_irqrestore(shost->host_lock, flags);
> +	scsi_clear_async_scan(shost);
>  
>  	mutex_unlock(&shost->scan_mutex);
>  


-- 
Damien Le Moal
Western Digital Research