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

[Damien Le Moal <dlemoal@kernel.org>]

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