From: Marcin Ślusarz <[email protected]>
Right now it's possible to hit NULL pointer dereference in
rtw_rx_fill_rx_status on hw object and/or its fields because
initialization routine can start getting USB replies before
rtw_dev is fully setup.
The stack trace looks like this:
rtw_rx_fill_rx_status
rtw8821c_query_rx_desc
rtw_usb_rx_handler
..
queue_work
rtw_usb_read_port_complete
..
usb_submit_urb
rtw_usb_rx_resubmit
rtw_usb_init_rx
rtw_usb_probe
So while we do the async stuff rtw_usb_probe continues and calls
rtw_register_hw, which does all kinds of initialization (e.g.
via ieee80211_register_hw) that rtw_rx_fill_rx_status relies on.
Fix this by moving the first usb_submit_urb after everything
is set up.
For me, this bug manifested as:
[ 8.893177] rtw_8821cu 1-1:1.2: band wrong, packet dropped
[ 8.910904] rtw_8821cu 1-1:1.2: hw->conf.chandef.chan NULL in rtw_rx_fill_rx_status
because I'm using Larry's backport of rtw88 driver with the NULL
checks in rtw_rx_fill_rx_status.
This should fix:
https://marc.info/?l=linux-wireless&m=167907688311943&w=2
Signed-off-by: Marcin Ślusarz <[email protected]>
Cc: Ping-Ke Shih <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/net/wireless/realtek/rtw88/usb.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index a0188511099a..98f81e3ae13e 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -740,7 +740,6 @@ static struct rtw_hci_ops rtw_usb_ops = {
static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
{
struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
- int i;
rtwusb->rxwq = create_singlethread_workqueue("rtw88_usb: rx wq");
if (!rtwusb->rxwq) {
@@ -752,13 +751,19 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
INIT_WORK(&rtwusb->rx_work, rtw_usb_rx_handler);
+ return 0;
+}
+
+static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
+{
+ struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+ int i;
+
for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
rtw_usb_rx_resubmit(rtwusb, rxcb);
}
-
- return 0;
}
static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
@@ -895,6 +900,8 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
goto err_destroy_rxwq;
}
+ rtw_usb_setup_rx(rtwdev);
+
return 0;
err_destroy_rxwq:
--
2.25.1
Kalle Valo <[email protected]> wrote:
> [email protected] writes:
>
> > From: Marcin Ślusarz <[email protected]>
> >
> > Right now it's possible to hit NULL pointer dereference in
> > rtw_rx_fill_rx_status on hw object and/or its fields because
> > initialization routine can start getting USB replies before
> > rtw_dev is fully setup.
> >
> > The stack trace looks like this:
> >
> > rtw_rx_fill_rx_status
> > rtw8821c_query_rx_desc
> > rtw_usb_rx_handler
> > ...
> > queue_work
> > rtw_usb_read_port_complete
> > ...
> > usb_submit_urb
> > rtw_usb_rx_resubmit
> > rtw_usb_init_rx
> > rtw_usb_probe
> >
> > So while we do the async stuff rtw_usb_probe continues and calls
> > rtw_register_hw, which does all kinds of initialization (e.g.
> > via ieee80211_register_hw) that rtw_rx_fill_rx_status relies on.
> >
> > Fix this by moving the first usb_submit_urb after everything
> > is set up.
> >
> > For me, this bug manifested as:
> > [ 8.893177] rtw_8821cu 1-1:1.2: band wrong, packet dropped
> > [ 8.910904] rtw_8821cu 1-1:1.2: hw->conf.chandef.chan NULL in rtw_rx_fill_rx_status
> > because I'm using Larry's backport of rtw88 driver with the NULL
> > checks in rtw_rx_fill_rx_status.
> >
> > This should fix:
> > https://marc.info/?l=linux-wireless&m=167907688311943&w=2
>
> Please use permalinks from lore.kernel.org, marc links don't contain the
> Message-Id and are hence unreliable.
>
Use tags below:
Reported-by: Tim K <[email protected]>
Closes: https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.com/
From: Marcin Ślusarz <[email protected]>
Right now it's possible to hit NULL pointer dereference in
rtw_rx_fill_rx_status on hw object and/or its fields because
initialization routine can start getting USB replies before
rtw_dev is fully setup.
The stack trace looks like this:
rtw_rx_fill_rx_status
rtw8821c_query_rx_desc
rtw_usb_rx_handler
..
queue_work
rtw_usb_read_port_complete
..
usb_submit_urb
rtw_usb_rx_resubmit
rtw_usb_init_rx
rtw_usb_probe
So while we do the async stuff rtw_usb_probe continues and calls
rtw_register_hw, which does all kinds of initialization (e.g.
via ieee80211_register_hw) that rtw_rx_fill_rx_status relies on.
Fix this by moving the first usb_submit_urb after everything
is set up.
For me, this bug manifested as:
[ 8.893177] rtw_8821cu 1-1:1.2: band wrong, packet dropped
[ 8.910904] rtw_8821cu 1-1:1.2: hw->conf.chandef.chan NULL in rtw_rx_fill_rx_status
because I'm using Larry's backport of rtw88 driver with the NULL
checks in rtw_rx_fill_rx_status.
Reported-by: Tim K <[email protected]>
Closes: https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.com/
Signed-off-by: Marcin Ślusarz <[email protected]>
Cc: Ping-Ke Shih <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/net/wireless/realtek/rtw88/usb.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index a0188511099a..98f81e3ae13e 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -740,7 +740,6 @@ static struct rtw_hci_ops rtw_usb_ops = {
static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
{
struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
- int i;
rtwusb->rxwq = create_singlethread_workqueue("rtw88_usb: rx wq");
if (!rtwusb->rxwq) {
@@ -752,13 +751,19 @@ static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
INIT_WORK(&rtwusb->rx_work, rtw_usb_rx_handler);
+ return 0;
+}
+
+static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
+{
+ struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+ int i;
+
for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
rtw_usb_rx_resubmit(rtwusb, rxcb);
}
-
- return 0;
}
static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
@@ -895,6 +900,8 @@ int rtw_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
goto err_destroy_rxwq;
}
+ rtw_usb_setup_rx(rtwdev);
+
return 0;
err_destroy_rxwq:
--
2.25.1
Tim K <[email protected]> wrote:
>
> > I gave this suggestions too early, since we have not gotten test result from Tim.
> > I will change them to "Link:" if no ACK from Tim while merging.
>
> Hey all, thanks for reaching out!
>
> Sadly I'm not able to work on this project right now, but I've
> forwarded this email to a few colleagues to bring them in the loop.
> Did you have a timeline you were looking at to close this off?
Thanks for your reply. I would take this patch to 6.11, so you have about
4-5 weeks. Is it enough to you?
Ping-Ke