From: Chengfeng Ye <dg573847474@gmail.com>

Fix a data race where sysfs read pm8001_ctl_host_sas_address_show() reads
pm8001_ha->sas_addr without synchronization while it can be written
from interrupt context in pm8001_mpi_get_nvmd_resp().

The write path is already protected by pm8001_ha->lock (held by
process_oq() when calling pm8001_mpi_get_nvmd_resp()),
but the sysfs read path accesses the 8-byte SAS address without
any synchronization, allowing torn reads.

Thread interleaving scenario:

           Thread A (sysfs read)     |    Thread B (interrupt context)
-------------------------------------+------------------------------------
pm8001_ctl_host_sas_address_show()  |
|- read sas_addr[0..3]               |
                                     | process_oq()
                                     | |- spin_lock_irqsave(&lock)
                                     | |- process_one_iomb()
                                     | |  |- pm8001_mpi_get_nvmd_resp()
                                     | |     |- memcpy(sas_addr, new, 8)
                                     | |        /* writes all 8 bytes */
                                     | |- spin_unlock_irqrestore(&lock)
|- read sas_addr[4..7]               |
   /* gets mix of old and new */    |

Fix by protecting the sysfs read with the same pm8001_ha->lock.

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index cbfda8c04e95..e49f11969b3b 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -311,8 +311,15 @@ static ssize_t pm8001_ctl_host_sas_address_show(struct device *cdev,
 	struct Scsi_Host *shost = class_to_shost(cdev);
 	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
 	struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
-	return sysfs_emit(buf, "0x%016llx\n",
-			be64_to_cpu(*(__be64 *)pm8001_ha->sas_addr));
+	unsigned long flags;
+	ssize_t ret;
+
+	spin_lock_irqsave(&pm8001_ha->lock, flags);
+	ret = sysfs_emit(buf, "0x%016llx\n",
+			 be64_to_cpu(*(__be64 *)pm8001_ha->sas_addr));
+	spin_unlock_irqrestore(&pm8001_ha->lock, flags);
+
+	return ret;
 }
 static DEVICE_ATTR(host_sas_address, S_IRUGO,
 		   pm8001_ctl_host_sas_address_show, NULL);
-- 
2.25.1

On 1/15/26 9:11 AM, Chengfeng Ye wrote:
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index cbfda8c04e95..e49f11969b3b 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -311,8 +311,15 @@ static ssize_t pm8001_ctl_host_sas_address_show(struct device *cdev,
>   	struct Scsi_Host *shost = class_to_shost(cdev);
>   	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
>   	struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
> -	return sysfs_emit(buf, "0x%016llx\n",
> -			be64_to_cpu(*(__be64 *)pm8001_ha->sas_addr));
> +	unsigned long flags;
> +	ssize_t ret;
> +
> +	spin_lock_irqsave(&pm8001_ha->lock, flags);
> +	ret = sysfs_emit(buf, "0x%016llx\n",
> +			 be64_to_cpu(*(__be64 *)pm8001_ha->sas_addr));
> +	spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> +
> +	return ret;
>   }
>   static DEVICE_ATTR(host_sas_address, S_IRUGO,
>   		   pm8001_ctl_host_sas_address_show, NULL);

Why isn't READ_ONCE() sufficient? And why explicit spin_lock_irqsave() 
and spin_unlock_irqrestore() calls instead of using scoped_guard()?

Bart.

On Thu, 2026-01-15 at 09:24 -0800, Bart Van Assche wrote:
> On 1/15/26 9:11 AM, Chengfeng Ye wrote:
> > diff --git a/drivers/scsi/pm8001/pm8001_ctl.c
> > b/drivers/scsi/pm8001/pm8001_ctl.c
> > index cbfda8c04e95..e49f11969b3b 100644
> > --- a/drivers/scsi/pm8001/pm8001_ctl.c
> > +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> > @@ -311,8 +311,15 @@ static ssize_t
> > pm8001_ctl_host_sas_address_show(struct device *cdev,
> >   	struct Scsi_Host *shost = class_to_shost(cdev);
> >   	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> >   	struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
> > -	return sysfs_emit(buf, "0x%016llx\n",
> > -			be64_to_cpu(*(__be64 *)pm8001_ha-
> > >sas_addr));
> > +	unsigned long flags;
> > +	ssize_t ret;
> > +
> > +	spin_lock_irqsave(&pm8001_ha->lock, flags);
> > +	ret = sysfs_emit(buf, "0x%016llx\n",
> > +			 be64_to_cpu(*(__be64 *)pm8001_ha-
> > >sas_addr));
> > +	spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> > +
> > +	return ret;
> >   }
> >   static DEVICE_ATTR(host_sas_address, S_IRUGO,
> >   		   pm8001_ctl_host_sas_address_show, NULL);
> 
> Why isn't READ_ONCE() sufficient? And why explicit
> spin_lock_irqsave() and spin_unlock_irqrestore() calls instead of
> using scoped_guard()?

Do we really care?  The host sas address isn't something that changes
often, so even if it is being overwritten in an interrupt routine, it's
likely with the same value.  Whereas taking the internal host lock in a
sysfs read routine could potentially be a DoS vector.

Regards,

James

Hi Bart,

Thanks for looking into this.

> Why isn't READ_ONCE() sufficient?
READ_ONCE() does not suffice because the write path still uses
memcpy(), which is not atomic.
So it is possible that when READ_ONCE() executes and reads all the 8
bytes at once, the SAS
address is still partially updated by the ISR.

> And why explicit spin_lock_irqsave()
> and spin_unlock_irqrestore() calls instead of using scoped_guard()?
Thanks for the notice. I just sent a v2 patch to address it.

Best,
Chengfeng