2023-06-28 02:25:58

by Tuo Li

Subject: [PATCH] wifi: plfxlc: Fix possible data races in rx_urb_complete()

The variable tx->submitted_urbs is often protected by the lock tx->lock
when is accessed. Here is an example in plfxlc_usb_disable_tx():

spin_lock_irqsave(&tx->lock, flags);
WARN_ON(!skb_queue_empty(&tx->submitted_skbs));
WARN_ON(tx->submitted_urbs != 0);
tx->submitted_urbs = 0;
spin_unlock_irqrestore(&tx->lock, flags);

However, it is accessed without holding the lock tx->lock in
rx_urb_complete():

if (tx->submitted_urbs++ < PURELIFI_URB_RETRY_MAX) --> Line 108
tx->submitted_urbs++ --> Line 110
tx->submitted_urbs = 0 --> Line 114

And thus data races can occur.
To fix these possible data races, a lock and unlock pair is added when
accessing the variable tx->submitted_urbs in rx_urb_complete().

Reported-by: BassCheck <[email protected]>
Signed-off-by: Tuo Li <[email protected]>
---
drivers/net/wireless/purelifi/plfxlc/usb.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/purelifi/plfxlc/usb.c b/drivers/net/wireless/purelifi/plfxlc/usb.c
index 76d0a778636a..44152a615c0e 100644
--- a/drivers/net/wireless/purelifi/plfxlc/usb.c
+++ b/drivers/net/wireless/purelifi/plfxlc/usb.c
@@ -69,6 +69,7 @@ static void handle_rx_packet(struct plfxlc_usb *usb, const u8 *buffer,
static void rx_urb_complete(struct urb *urb)
{
struct plfxlc_usb_tx *tx;
+ unsigned long flags;
struct plfxlc_usb *usb;
unsigned int length;
const u8 *buffer;
@@ -105,13 +106,16 @@ static void rx_urb_complete(struct urb *urb)
return;
default:
dev_dbg(plfxlc_urb_dev(urb), "urb %p error %d\n", urb, urb->status);
+ spin_lock_irqsave(&tx->lock, flags);
if (tx->submitted_urbs++ < PURELIFI_URB_RETRY_MAX) {
dev_dbg(plfxlc_urb_dev(urb), "urb %p resubmit %d", urb,
tx->submitted_urbs++);
+ spin_unlock_irqrestore(&tx->lock, flags);
goto resubmit;
} else {
dev_dbg(plfxlc_urb_dev(urb), "urb %p max resubmits reached", urb);
tx->submitted_urbs = 0;
+ spin_unlock_irqrestore(&tx->lock, flags);
return;
}
}
--
2.34.1

2023-06-28 08:13:16

by Dmitry Antipov

Subject: Re: [PATCH] wifi: plfxlc: Fix possible data races in rx_urb_complete()

On 6/28/23 05:21, Tuo Li wrote:

> if (tx->submitted_urbs++ < PURELIFI_URB_RETRY_MAX) {
> dev_dbg(plfxlc_urb_dev(urb), "urb %p resubmit %d", urb,
> tx->submitted_urbs++);

BTW, doing 'tx->submitted_urbs++' inside a debugging statement is most likely
a bad idea.

Dmitry