[PATCH] wifi: rtw88: schedule rx work after everything is set up

[PATCH] wifi: rtw88: schedule rx work after everything is set up

[marcin.slusarz]

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

Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
Cc: Ping-Ke Shih <pkshih@realtek.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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

Re: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

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/

RE: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

+ 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/
> 

[PATCH] wifi: rtw88: schedule rx work after everything is set up

[Marcin Ślusarz]

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.

Reported-by: Tim K <tpkuester@gmail.com>
Closes: https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.com/
Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
Cc: Ping-Ke Shih <pkshih@realtek.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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

RE: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

Marcin Ślusarz <marcin.slusarz@gmail.com> wrote:
> From: Marcin Ślusarz <mslusarz@renau.com>
> 
> Reported-by: Tim K <tpkuester@gmail.com>
> Closes:
> https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.
> com/

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. 

> Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> Cc: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

This is v2 version, so mail subject should be "[PATCH v2] ....", and add 
change log here, like:

---  (delimiter is important here)

v2: add Reported-by and Closes.

> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 

Also I would prefer to point out "usb" in subject, please use "wifi: rtw88: usb: "
as prefix.

[...]

Re: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Tim K]

> 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?

- Tim

RE: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

Tim K <tpkuester@gmail.com> 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

RE: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

Hi Tim,

Ping-Ke Shih wrote:
> Tim K <tpkuester@gmail.com> 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?

Do you have any update on this patch? 

Ping-Ke

Re: [PATCH] wifi: rtw88: schedule rx work after everything is set up

[Ping-Ke Shih]

On Tue, 2024-05-28 at 13:02 +0200, Marcin Ślusarz wrote:
> 
> 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.
> 
> Reported-by: Tim K <tpkuester@gmail.com>
> Closes: 
> https://lore.kernel.org/linux-wireless/CA+shoWQ7P49jhQasofDcTdQhiuarPTjYEDa--NiVVx494WcuQw@mail.gmail.com/

Change Closes to Link during committing because of no ACK from Tim.

> Signed-off-by: Marcin Ślusarz <mslusarz@renau.com>
> Cc: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 

1 patch(es) applied to rtw-next branch of rtw.git, thanks.

adc539784c98 wifi: rtw88: usb: schedule rx work after everything is set up

---
https://github.com/pkshih/rtw.git