Skip to contentSkip to conversation

ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

PATCH In review Rosen Penev 2 participants 2 messages Sep 8 – Sep 9 libata

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

hsdev->sactive_issued is written locklessly in sata_dwc_isr() before acquiring host->lock, while sata_dwc_qc_complete() performs a read-modify-write on the same field under the lock. This creates a data race that can corrupt NCQ tag tracking state.

Move the zero assignment inside the critical section so all accesses to sactive_issued are serialized by host->lock.

Fixes 2d20da00c324b ("ata: sata_dwc_460ex: get rid of global data")
Assisted-by opencode:big-pickle

---
v2: resend as standalone patch

 drivers/ata/sata_dwc_460ex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/sata_dwc_460ex.c +1 -1
index 8a1d80ac906a..73bacdfd0bd3 100644
@@ -465,9 +465,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 	int handled, port = 0;
 	uint intpr, sactive, sactive2, tag_mask;
 	struct sata_dwc_device_port *hsdevp;
-	hsdev->sactive_issued = 0;
 
 	spin_lock_irqsave(&host->lock, flags);
+	hsdev->sactive_issued = 0;
 
 	/* Read the interrupt register */
 	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);
Signature · 1 line
2.55.0
maintainer
#1 lore raw
Quoted 9 lines · Rosen Penev

hsdev->sactive_issued is written locklessly in sata_dwc_isr() before acquiring host->lock, while sata_dwc_qc_complete() performs a read-modify-write on the same field under the lock. This creates a data race that can corrupt NCQ tag tracking state.

Move the zero assignment inside the critical section so all accesses to sactive_issued are serialized by host->lock.

Fixes 2d20da00c324b ("ata: sata_dwc_460ex: get rid of global data")

Fixes tag seems wrong.

Commit in fixes did:

- host_pvt.sata_dwc_sactive_issued = 0;
+ hsdev->sactive_issued = 0;

        spin_lock_irqsave(&host->lock, flags);

ie. it simply did a rename, so sactive_issued was assigned to zero before taking the lock, both before and after the commit in Fixes:.

Kind regards,
Niklas