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

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

Posted by Chaohai Chen 1 month, 1 week ago

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@aliyun.com>
---
 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;
+}
+
+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);
 
-- 
2.43.7

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

Posted by Christoph Hellwig 1 month ago

No, using a lock to read a single scalar value makes zero sense.
Just switch to READ_ONCE/WRITE_ONCE to make alpha and KCSAN happy.

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

Posted by James Bottomley 1 month ago

On Tue, 2026-03-03 at 07:12 -0800, Christoph Hellwig wrote:
> No, using a lock to read a single scalar value makes zero sense.
> Just switch to READ_ONCE/WRITE_ONCE to make alpha and KCSAN happy.

Fully agreed here.  Although bool is width undefined, it's bound to be
stored in a naturally aligned architectural type, which will be atomic
for read and write anyway, so the macros will just make sure the
compiler does the right thing.

Regards,

James

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

Posted by Damien Le Moal 1 month, 1 week ago

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@aliyun.com>
> ---
>  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

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

Posted by Chaohai Chen 1 month, 1 week ago

On Tue, Mar 03, 2026 at 05:45:13PM +0900, Damien Le Moal wrote:
> 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@aliyun.com>
> > ---
> >  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 ?
> 
The structure member async_stcan is defined in a bit field manner, 
and using atomic may change the structure definition, making it too complex

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

Posted by Damien Le Moal 1 month, 1 week ago

On 3/3/26 18:20, Chaohai Chen wrote:
> On Tue, Mar 03, 2026 at 05:45:13PM +0900, Damien Le Moal wrote:
>> 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@aliyun.com>
>>> ---
>>>  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 ?
>>
> The structure member async_stcan is defined in a bit field manner, 
> and using atomic may change the structure definition, making it too complex

But the spinlock is useless here...

lock
copy async_scan
unlock

test using the copy

That is not atomic at all, and does not need to be. So all the spinlock is doing
is to avoid "getting garbage" because another bit around it is being changed.
Using an atomic would be far simpler and cleaner. I do not see any complexity at
all with that. And that would not even grow the Scsi_Host structure size: use
pahole and you can see that there are 4 Bytes holes in there.

-- 
Damien Le Moal
Western Digital Research

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

Posted by Bart Van Assche 1 month, 1 week ago

On 3/2/26 4:13 AM, Chaohai Chen wrote:
> +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;
> +}

Please use scoped_guard(), e.g. as follows:

static bool scsi_scan_async(struct Scsi_Host *shost)
{
	scoped_guard(spinlock_irqsave, shost->host_lock)
		return shost->async_scan;
}

> +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);
> +}
Please drop the scsi_set_async_scan() and scsi_clear_async_scan()
functions since these only have one caller.

Thanks,

Bart.