| Message ID | 20240527170137.455671-1-marcin.slusarz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtw88: schedule rx work after everything is set up | expand |
marcin.slusarz@gmail.com writes: > From: Marcin Ślusarz <mslusarz@renau.com> > > 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.
Kalle Valo <kvalo@kernel.org> wrote: > marcin.slusarz@gmail.com writes: > > > From: Marcin Ślusarz <mslusarz@renau.com> > > > > 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 <tpkuester@gmail.com> Closes: https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.com/
+ Tim K (reporter) Hi Tim, Could you please try this patch can help your rtw8822bu? Ping-Ke Shih <pkshih@realtek.com> wrote: > Kalle Valo <kvalo@kernel.org> wrote: > > marcin.slusarz@gmail.com writes: > > > > > From: Marcin Ślusarz <mslusarz@renau.com> > > > > > > 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 <tpkuester@gmail.com> > Closes: > https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail. > com/ >
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: