Re: [PATCH] scsi: libsas: Fix dev_list race conditions with proper locking

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

On 2/2/26 16:36, Chaohai Chen wrote:
> On Mon, Feb 02, 2026 at 10:21:43AM +0900, Damien Le Moal wrote:
>> On 1/31/26 20:19, Chaohai Chen wrote:
>>>>> +	 * We need to unlock before calling sas_unregister_dev() as it
>>>>> +	 * may sleep, but we hold a reference to prevent device removal.
>>>>
>>>> And why is that necessary ?
>>>>
>>> Because when unlocked, it is possible that the device has already been 
>>> released by another thread. If there is no reference count, it will lead
>>> to used after free.
>>
>> Please clearly explain the problem path. Your statements about "another thread"
>> is too vague.
>>
> 1.
> CPU 1: disco_q                          CPU 2: event_q
> ==============================          ==============================
> sas_discover_domain()                   sas_phye_loss_of_signal()
> 
> sas_ex_level_discovery()                sas_deform_port(phy, true)
> 
> list_for_each_entry(dev,                sas_unregister_domain_devices()
>   &port->dev_list, ...)
> 
> NOP                                     list_for_each_entry_safe_reverse(
>                                         dev, n, &port->dev_list, ...)
> 
> NOP                                     sas_unregister_dev(port, dev)
> 
> NOP                                     kfree(dev)
> 
> if (dev_is_expander(dev->dev_type))(UAF)
> ...
> > 2.
> CPU 1: disco_q                          CPU 2: event_q
> ==============================          ==============================
> sas_resume_devices()                    sas_porte_link_reset_err()
> 
> sas_resume_port()                       sas_deform_port(phy, true)
> 
> list_for_each_entry_safe(dev,           sas_unregister_domain_devices()
>   &port->dev_list, ...)
> 
> NOP                                     free dev
> 
> visit dev->ex_dev(UAF)

OK. Then add this to the commit message to make everything clear. Also, the
examples above are only for device removal, and given that
list_for_each_entry_safe() is safe against removals, that is likely the only
change needed (in case 1, disco_q side), together with a device get/put inside
the loop. Not sure that the spinlock to protect the list is actually needed.


-- 
Damien Le Moal
Western Digital Research