| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aX3lV4erBYL068PT@LAPTOP-RK2E6KJ3.localdomain>
Date: Sat, 31 Jan 2026 19:19:51 +0800
From: Chaohai Chen <wdhh6@...yun.com>
To: Damien Le Moal <dlemoal@...nel.org>
Cc: john.g.garry@...cle.com, yanaijie@...wei.com,
James.Bottomley@...senpartnership.com, martin.petersen@...cle.com,
johannes.thumshirn@....com, mingo@...nel.org, cassel@...nel.org,
tglx@...nel.org, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: libsas: Fix dev_list race conditions with proper
locking
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@...yun.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
Powered by blists - more mailing lists