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

[Chaohai Chen <wdhh6@aliyun.com>]

On Fri, Jan 30, 2026 at 12:02:01PM +0900, Damien Le Moal wrote:
> On 1/29/26 18:38, Chaohai Chen wrote:
> > Multiple functions in libsas were accessing port->dev_list without
> > proper locking, leading to potential race conditions that could cause:
> > - Use-after-free when devices are removed during list traversal
> > - List corruption from concurrent modifications
> > - System crashes from accessing freed memory
> 
> There si a lot going on in this patch with reference counting changes that are
> not very clear. Ideally, this patch should be split to address reference
> counting and exclusive access to the list with a spinlock.
> 
> More comments below.
> 
> > 
> > This patch adds proper dev_list_lock protection to the following functions:
> > 
> > 1. sas_ex_level_discovery(): Added locking around list traversal with
> >    safe iteration and reference counting for devices. The lock is
> >    released before calling functions that may sleep
> >    (sas_ex_discover_devices).
> > 
> > 2. sas_dev_present_in_domain(): Added locking for read-only list access
> >    to prevent reading inconsistent list state.
> > 
> > 3. sas_suspend_devices(): Added locking around list traversal to prevent
> >    concurrent modifications during device suspension.
> > 
> > 4. sas_unregister_domain_devices(): Added proper locking with reference
> >    counting. The lock is released before calling sas_unregister_dev()
> >    which may sleep, but device references are held to prevent premature
> >    removal.
> > 
> > 5. sas_port_event_worker(): Added locking around list traversal with
> >    reference counting for devices accessed outside the lock.
> > 
> > All modifications follow the pattern of:
> > - Hold dev_list_lock during list traversal
> > - Use list_for_each_entry_safe where list may be modified
> > - Take device reference (kref_get) before releasing lock
> > - Release lock before calling functions that may sleep
> > - Release device reference (sas_put_device) after use
> > 
> > Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
> > ---
> >  drivers/scsi/libsas/sas_discover.c | 14 +++++++++++++
> >  drivers/scsi/libsas/sas_expander.c | 32 +++++++++++++++++++++++-------
> >  drivers/scsi/libsas/sas_port.c     | 10 ++++++++++
> >  3 files changed, 49 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index b07062db50b2..3c18fdfde8c2 100644
> > --- a/drivers/scsi/libsas/sas_discover.c
> > +++ b/drivers/scsi/libsas/sas_discover.c
> > @@ -245,8 +245,10 @@ static void sas_suspend_devices(struct work_struct *work)
> >  	 * suspension, we force the issue here to keep the reference
> >  	 * counts aligned
> >  	 */
> > +	spin_lock_irq(&port->dev_list_lock);
> >  	list_for_each_entry(dev, &port->dev_list, dev_list_node)
> >  		sas_notify_lldd_dev_gone(dev);
> > +	spin_unlock_irq(&port->dev_list_lock);
> >  
> >  	/* we are suspending, so we know events are disabled and
> >  	 * phy_list is not being mutated
> > @@ -410,11 +412,23 @@ void sas_unregister_domain_devices(struct asd_sas_port *port, bool gone)
> >  {
> >  	struct domain_device *dev, *n;
> >  
> > +	/* Lock while iterating to prevent concurrent modifications.
> 
> Please use the correct kernel style: multi-line comments start with a "/*" line
> with no text.
> 
> > +	 * 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.
> > +	 */
> > +	spin_lock_irq(&port->dev_list_lock);
> >  	list_for_each_entry_safe_reverse(dev, n, &port->dev_list, dev_list_node) {
> >  		if (gone)
> >  			set_bit(SAS_DEV_GONE, &dev->state);
> > +		kref_get(&dev->kref);
> > +		spin_unlock_irq(&port->dev_list_lock);
> > +
> >  		sas_unregister_dev(port, dev);
> > +		sas_put_device(dev);
> > +
> > +		spin_lock_irq(&port->dev_list_lock);
> >  	}
> > +	spin_unlock_irq(&port->dev_list_lock);
> >  
> >  	list_for_each_entry_safe(dev, n, &port->disco_list, disco_list_node)
> >  		sas_unregister_dev(port, dev);
> > diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> > index d953225f6cc2..c82c9b3d5103 100644
> > --- a/drivers/scsi/libsas/sas_expander.c
> > +++ b/drivers/scsi/libsas/sas_expander.c
> > @@ -643,14 +643,21 @@ static int sas_dev_present_in_domain(struct asd_sas_port *port,
> >  					    u8 *sas_addr)
> >  {
> >  	struct domain_device *dev;
> > +	int found = 0;
> 
> Use a bool please. That menas changing the function to return a bool as well.
> 
> >  
> >  	if (SAS_ADDR(port->sas_addr) == SAS_ADDR(sas_addr))
> >  		return 1;
> > +
> > +	spin_lock_irq(&port->dev_list_lock);
> >  	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
> > -		if (SAS_ADDR(dev->sas_addr) == SAS_ADDR(sas_addr))
> > -			return 1;
> > +		if (SAS_ADDR(dev->sas_addr) == SAS_ADDR(sas_addr)) {
> > +			found = 1;
> > +			break;
> > +		}
> >  	}
> > -	return 0;
> > +	spin_unlock_irq(&port->dev_list_lock);
> > +
> > +	return found;
> >  }
> >  
> >  #define RPEL_REQ_SIZE	16
> > @@ -1579,20 +1586,31 @@ static int sas_discover_expander(struct domain_device *dev)
> >  static int sas_ex_level_discovery(struct asd_sas_port *port, const int level)
> >  {
> >  	int res = 0;
> > -	struct domain_device *dev;
> > +	struct domain_device *dev, *n;
> >  
> > -	list_for_each_entry(dev, &port->dev_list, dev_list_node) {
> > +	spin_lock_irq(&port->dev_list_lock);
> > +	list_for_each_entry_safe(dev, n, &port->dev_list, dev_list_node) {
> >  		if (dev_is_expander(dev->dev_type)) {
> >  			struct sas_expander_device *ex =
> >  				rphy_to_expander_device(dev->rphy);
> >  
> > -			if (level == ex->level)
> > +			if (level == ex->level) {
> > +				kref_get(&dev->kref);
> > +				spin_unlock_irq(&port->dev_list_lock);
This will be unlocked here, so there will be no deadlock below.
> >  				res = sas_ex_discover_devices(dev, -1);
> > -			else if (level > 0)
> > +				sas_put_device(dev);
> > +				spin_lock_irq(&port->dev_list_lock);
> 
> This was locked already before the loop. So you will deadlock here... no ?
> 
No deadlock here.
> > +			} else if (level > 0) {
> > +				kref_get(&port->port_dev->kref);
> > +				spin_unlock_irq(&port->dev_list_lock);
> >  				res = sas_ex_discover_devices(port->port_dev, -1);
> > +				sas_put_device(port->port_dev);
> > +				spin_lock_irq(&port->dev_list_lock);
> > +			}
> >  
> >  		}
> >  	}
> > +	spin_unlock_irq(&port->dev_list_lock);
> >  
> >  	return res;
> >  }
> > diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
> > index de7556070048..491c9f7104c6 100644
> > --- a/drivers/scsi/libsas/sas_port.c
> > +++ b/drivers/scsi/libsas/sas_port.c
> > @@ -44,13 +44,19 @@ static void sas_resume_port(struct asd_sas_phy *phy)
> >  	 * 1/ presume every device came back
> >  	 * 2/ force the next revalidation to check all expander phys
> >  	 */
> > +	spin_lock_irq(&port->dev_list_lock);
> >  	list_for_each_entry_safe(dev, n, &port->dev_list, dev_list_node) {
> >  		int i, rc;
> >  
> > +		kref_get(&dev->kref);
> > +		spin_unlock_irq(&port->dev_list_lock);
> > +
> >  		rc = sas_notify_lldd_dev_found(dev);
> >  		if (rc) {
> >  			sas_unregister_dev(port, dev);
> >  			sas_destruct_devices(port);
> > +			sas_put_device(dev);
> 
> Same here. Why is the extra reference count needed ?
> 

The use of reference counting is to unlock the logic for device discovery 
and exception handling which may involve scheduling. There cannot be 
scheduling inside the spin lock.
> > +			spin_lock_irq(&port->dev_list_lock);
> >  			continue;
> >  		}
> >  
> > @@ -62,7 +68,11 @@ static void sas_resume_port(struct asd_sas_phy *phy)
> >  				phy->phy_change_count = -1;
> >  			}
> >  		}
> > +
> > +		sas_put_device(dev);
> > +		spin_lock_irq(&port->dev_list_lock);
> >  	}
> > +	spin_unlock_irq(&port->dev_list_lock);
> >  
> >  	sas_discover_event(port, DISCE_RESUME);
> >  }
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research


--
Chaohai Chen