diff mbox series

[v2] usb: host: oxu210hp-hcd: Fix potential deadlock on &oxu->mem_lock

Message ID 20230729092634.78336-1-dg573847474@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] usb: host: oxu210hp-hcd: Fix potential deadlock on &oxu->mem_lock | expand

Commit Message

Chengfeng Ye July 29, 2023, 9:26 a.m. UTC 
&oxu->mem_lock is acquired by isr oxu_irq() along the below call
chain under hardirq context.

<hard interrupt>
        -> oxu_irq()
        -> oxu210_hcd_irq()
        -> ehci_work()
        -> scan_async()
        -> qh_completions()
        -> oxu_murb_free()
        -> spin_lock(&oxu->mem_lock)

Thus the acquisition of the lock under process context should disable
irq, otherwise deadlock could happen if the irq happens to preempt the
execution while the lock is held in process context on the same CPU.

This flaw was found by an experimental static analysis tool I am developing
for irq-related deadlock. x86_64 allmodconfig using gcc shows no new
warning.

The patch fixes the potential deadlocks by using spin_lock_irqsave() on
&oxu->mem_lock

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>

Changes in v2
- use spin_lock_irqsave() on more potential deadlock sites of &oxu->mem_lock
---
 drivers/usb/host/oxu210hp-hcd.c | 42 ++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

Comments

Greg KH Aug. 8, 2023, 8:28 a.m. UTC | #1 
On Sat, Jul 29, 2023 at 09:26:34AM +0000, Chengfeng Ye wrote:
> &oxu->mem_lock is acquired by isr oxu_irq() along the below call
> chain under hardirq context.
> 
> <hard interrupt>
>         -> oxu_irq()
>         -> oxu210_hcd_irq()
>         -> ehci_work()
>         -> scan_async()
>         -> qh_completions()
>         -> oxu_murb_free()
>         -> spin_lock(&oxu->mem_lock)
> 
> Thus the acquisition of the lock under process context should disable
> irq, otherwise deadlock could happen if the irq happens to preempt the
> execution while the lock is held in process context on the same CPU.
> 
> This flaw was found by an experimental static analysis tool I am developing
> for irq-related deadlock. x86_64 allmodconfig using gcc shows no new
> warning.
> 
> The patch fixes the potential deadlocks by using spin_lock_irqsave() on
> &oxu->mem_lock
> 
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
> 
> Changes in v2
> - use spin_lock_irqsave() on more potential deadlock sites of &oxu->mem_lock

This needs to be below the --- line, as documented, so it doesn't show
up in the changelog.

How did you test this change?  Do you have hardware to test it out?  If
not, I don't think we can accpet this, see the kernel documentation for
what we accept from tools and researchers.

thanks,

greg k-h
Chengfeng Ye Aug. 8, 2023, 3:17 p.m. UTC | #2 
Thank you for pointing out the formatting error in the patch.

Truly only building testing is performed as mentioned in commit but
no hardware testing was able to be performed. The bug was detected
by tool and also reviewed by myself, I thought it could be a real issue
so I send the patch to fix it by using safer irq-variant locking API.

I understand the guideline and appreciate it. Would be grateful if anyone
who has written/is familiar with the driver could review whether this is
a real bug and whether it should be fixed.

Best,
Chengfeng
diff mbox series

Patch

diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index 50c1ccabb0f5..83061204b033 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -908,6 +908,7 @@  static int oxu_buf_alloc(struct oxu_hcd *oxu, struct ehci_qtd *qtd, int len)
 {
 	int n_blocks;	/* minium blocks needed to hold len */
 	int a_blocks;	/* blocks allocated */
+	unsigned long flags;
 	int i, j;
 
 	/* Don't allocte bigger than supported */
@@ -916,7 +917,7 @@  static int oxu_buf_alloc(struct oxu_hcd *oxu, struct ehci_qtd *qtd, int len)
 		return -ENOMEM;
 	}
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	/* Number of blocks needed to hold len */
 	n_blocks = (len + BUFFER_SIZE - 1) / BUFFER_SIZE;
@@ -944,14 +945,14 @@  static int oxu_buf_alloc(struct oxu_hcd *oxu, struct ehci_qtd *qtd, int len)
 		qtd->qtd_buffer_len = BUFFER_SIZE * a_blocks;
 		oxu->db_used[i] = a_blocks;
 
-		spin_unlock(&oxu->mem_lock);
+		spin_unlock_irqrestore(&oxu->mem_lock, flags);
 
 		return 0;
 	}
 
 	/* Failed */
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 
 	return -ENOMEM;
 }
@@ -959,8 +960,9 @@  static int oxu_buf_alloc(struct oxu_hcd *oxu, struct ehci_qtd *qtd, int len)
 static void oxu_buf_free(struct oxu_hcd *oxu, struct ehci_qtd *qtd)
 {
 	int index;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	index = (qtd->buffer - (void *) &oxu->mem->db_pool[0])
 							 / BUFFER_SIZE;
@@ -969,7 +971,7 @@  static void oxu_buf_free(struct oxu_hcd *oxu, struct ehci_qtd *qtd)
 	qtd->buffer_dma = 0;
 	qtd->buffer = NULL;
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 }
 
 static inline void ehci_qtd_init(struct ehci_qtd *qtd, dma_addr_t dma)
@@ -985,24 +987,26 @@  static inline void ehci_qtd_init(struct ehci_qtd *qtd, dma_addr_t dma)
 static inline void oxu_qtd_free(struct oxu_hcd *oxu, struct ehci_qtd *qtd)
 {
 	int index;
+	unsigned long flags;
 
 	if (qtd->buffer)
 		oxu_buf_free(oxu, qtd);
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	index = qtd - &oxu->mem->qtd_pool[0];
 	oxu->qtd_used[index] = 0;
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 }
 
 static struct ehci_qtd *ehci_qtd_alloc(struct oxu_hcd *oxu)
 {
 	int i;
 	struct ehci_qtd *qtd = NULL;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	for (i = 0; i < QTD_NUM; i++)
 		if (!oxu->qtd_used[i])
@@ -1022,7 +1026,7 @@  static struct ehci_qtd *ehci_qtd_alloc(struct oxu_hcd *oxu)
 		oxu->qtd_used[i] = 1;
 	}
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 
 	return qtd;
 }
@@ -1030,13 +1034,14 @@  static struct ehci_qtd *ehci_qtd_alloc(struct oxu_hcd *oxu)
 static void oxu_qh_free(struct oxu_hcd *oxu, struct ehci_qh *qh)
 {
 	int index;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	index = qh - &oxu->mem->qh_pool[0];
 	oxu->qh_used[index] = 0;
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 }
 
 static void qh_destroy(struct kref *kref)
@@ -1058,8 +1063,9 @@  static struct ehci_qh *oxu_qh_alloc(struct oxu_hcd *oxu)
 {
 	int i;
 	struct ehci_qh *qh = NULL;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	for (i = 0; i < QHEAD_NUM; i++)
 		if (!oxu->qh_used[i])
@@ -1086,7 +1092,7 @@  static struct ehci_qh *oxu_qh_alloc(struct oxu_hcd *oxu)
 		oxu->qh_used[i] = 1;
 	}
 unlock:
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 
 	return qh;
 }
@@ -1106,13 +1112,14 @@  static inline void qh_put(struct ehci_qh *qh)
 static void oxu_murb_free(struct oxu_hcd *oxu, struct oxu_murb *murb)
 {
 	int index;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	index = murb - &oxu->murb_pool[0];
 	oxu->murb_used[index] = 0;
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 }
 
 static struct oxu_murb *oxu_murb_alloc(struct oxu_hcd *oxu)
@@ -1120,8 +1127,9 @@  static struct oxu_murb *oxu_murb_alloc(struct oxu_hcd *oxu)
 {
 	int i;
 	struct oxu_murb *murb = NULL;
+	unsigned long flags;
 
-	spin_lock(&oxu->mem_lock);
+	spin_lock_irqsave(&oxu->mem_lock, flags);
 
 	for (i = 0; i < MURB_NUM; i++)
 		if (!oxu->murb_used[i])
@@ -1133,7 +1141,7 @@  static struct oxu_murb *oxu_murb_alloc(struct oxu_hcd *oxu)
 		oxu->murb_used[i] = 1;
 	}
 
-	spin_unlock(&oxu->mem_lock);
+	spin_unlock_irqrestore(&oxu->mem_lock, flags);
 
 	return murb;
 }