* [PATCH] rtw88: fix potential NULL skb access in TX ISR
@ 2020-01-07 8:08 yhchuang
2020-01-07 10:40 ` Chris Chiu
2020-01-26 15:42 ` Kalle Valo
0 siblings, 2 replies; 5+ messages in thread
From: yhchuang @ 2020-01-07 8:08 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, briannorris, mikhail.v.gavrilov, rtereguloff
From: Yan-Hsuan Chuang <yhchuang@realtek.com>
Sometimes the TX queue may be empty and we could possible
dequeue a NULL pointer, crash the kernel. If the skb is NULL
then there is nothing to do, just leave the ISR.
And the TX queue should not be empty here, so print an error
to see if there is anything wrong for DMA ring.
Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index a58e8276a41a..a6746b5a9ff2 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
while (count--) {
skb = skb_dequeue(&ring->queue);
+ if (!skb) {
+ rtw_err(rtwdev, "failed to dequeue %d skb TX queue %d, BD=0x%08x, rp %d -> %d\n",
+ count, hw_queue, bd_idx, ring->r.rp, cur_rp);
+ break;
+ }
tx_data = rtw_pci_get_tx_data(skb);
pci_unmap_single(rtwpci->pdev, tx_data->dma, skb->len,
PCI_DMA_TODEVICE);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
2020-01-07 8:08 [PATCH] rtw88: fix potential NULL skb access in TX ISR yhchuang
@ 2020-01-07 10:40 ` Chris Chiu
2020-01-07 11:21 ` Tony Chuang
2020-01-26 15:42 ` Kalle Valo
1 sibling, 1 reply; 5+ messages in thread
From: Chris Chiu @ 2020-01-07 10:40 UTC (permalink / raw)
To: Tony Chuang
Cc: Kalle Valo, linux-wireless, Brian Norris, mikhail.v.gavrilov,
rtereguloff
On Tue, Jan 7, 2020 at 4:08 PM <yhchuang@realtek.com> wrote:
>
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Sometimes the TX queue may be empty and we could possible
> dequeue a NULL pointer, crash the kernel. If the skb is NULL
> then there is nothing to do, just leave the ISR.
>
> And the TX queue should not be empty here, so print an error
> to see if there is anything wrong for DMA ring.
>
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
> drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index a58e8276a41a..a6746b5a9ff2 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>
> while (count--) {
> skb = skb_dequeue(&ring->queue);
> + if (!skb) {
> + rtw_err(rtwdev, "failed to dequeue %d skb TX queue %d, BD=0x%08x, rp %d -> %d\n",
> + count, hw_queue, bd_idx, ring->r.rp, cur_rp);
> + break;
> + }
> tx_data = rtw_pci_get_tx_data(skb);
> pci_unmap_single(rtwpci->pdev, tx_data->dma, skb->len,
> PCI_DMA_TODEVICE);
> --
> 2.17.1
>
Maybe we can simply do 'while (count -- &&
!skb_queue_empty(&ring->queue))' to achieve the same thing?
I don't think it worths to raise an error unless the count is expected
to exactly match the queue length in any
circumstances.
Chris
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] rtw88: fix potential NULL skb access in TX ISR
2020-01-07 10:40 ` Chris Chiu
@ 2020-01-07 11:21 ` Tony Chuang
2020-01-09 10:26 ` Chris Chiu
0 siblings, 1 reply; 5+ messages in thread
From: Tony Chuang @ 2020-01-07 11:21 UTC (permalink / raw)
To: Chris Chiu
Cc: Kalle Valo, linux-wireless, Brian Norris,
mikhail.v.gavrilov@gmail.com, rtereguloff@gmail.com
From: Chris Chiu
> Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
>
> On Tue, Jan 7, 2020 at 4:08 PM <yhchuang@realtek.com> wrote:
> >
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > Sometimes the TX queue may be empty and we could possible
> > dequeue a NULL pointer, crash the kernel. If the skb is NULL
> > then there is nothing to do, just leave the ISR.
> >
> > And the TX queue should not be empty here, so print an error
> > to see if there is anything wrong for DMA ring.
> >
> > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > ---
> > drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> > index a58e8276a41a..a6746b5a9ff2 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev,
> struct rtw_pci *rtwpci,
> >
> > while (count--) {
> > skb = skb_dequeue(&ring->queue);
> > + if (!skb) {
> > + rtw_err(rtwdev, "failed to dequeue %d skb TX
> queue %d, BD=0x%08x, rp %d -> %d\n",
> > + count, hw_queue, bd_idx, ring->r.rp,
> cur_rp);
> > + break;
> > + }
> > tx_data = rtw_pci_get_tx_data(skb);
> > pci_unmap_single(rtwpci->pdev, tx_data->dma,
> skb->len,
> > PCI_DMA_TODEVICE);
> > --
> > 2.17.1
> >
>
> Maybe we can simply do 'while (count -- &&
> !skb_queue_empty(&ring->queue))' to achieve the same thing?
> I don't think it worths to raise an error unless the count is expected
> to exactly match the queue length in any
> circumstances.
>
Yes, I expected that the queue length should match with the DMA ring.
And so I printed an error to see why the count mismatched.
Yan-Hsuan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
2020-01-07 11:21 ` Tony Chuang
@ 2020-01-09 10:26 ` Chris Chiu
0 siblings, 0 replies; 5+ messages in thread
From: Chris Chiu @ 2020-01-09 10:26 UTC (permalink / raw)
To: Tony Chuang
Cc: Kalle Valo, linux-wireless, Brian Norris,
mikhail.v.gavrilov@gmail.com, rtereguloff@gmail.com
On Tue, Jan 7, 2020 at 7:21 PM Tony Chuang <yhchuang@realtek.com> wrote:
>
> From: Chris Chiu
> > Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
> >
> > On Tue, Jan 7, 2020 at 4:08 PM <yhchuang@realtek.com> wrote:
> > >
> > > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > >
> > > Sometimes the TX queue may be empty and we could possible
> > > dequeue a NULL pointer, crash the kernel. If the skb is NULL
> > > then there is nothing to do, just leave the ISR.
> > >
> > > And the TX queue should not be empty here, so print an error
> > > to see if there is anything wrong for DMA ring.
> > >
> > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > > ---
> > > drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > > index a58e8276a41a..a6746b5a9ff2 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > > @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > >
> > > while (count--) {
> > > skb = skb_dequeue(&ring->queue);
> > > + if (!skb) {
> > > + rtw_err(rtwdev, "failed to dequeue %d skb TX
> > queue %d, BD=0x%08x, rp %d -> %d\n",
> > > + count, hw_queue, bd_idx, ring->r.rp,
> > cur_rp);
> > > + break;
> > > + }
> > > tx_data = rtw_pci_get_tx_data(skb);
> > > pci_unmap_single(rtwpci->pdev, tx_data->dma,
> > skb->len,
> > > PCI_DMA_TODEVICE);
> > > --
> > > 2.17.1
> > >
> >
> > Maybe we can simply do 'while (count -- &&
> > !skb_queue_empty(&ring->queue))' to achieve the same thing?
> > I don't think it worths to raise an error unless the count is expected
> > to exactly match the queue length in any
> > circumstances.
> >
>
> Yes, I expected that the queue length should match with the DMA ring.
> And so I printed an error to see why the count mismatched.
>
> Yan-Hsuan
Maybe you can spin lock around skb_dequeue and skb_enqueue to prevent
some possible race conditions?
Chris
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
2020-01-07 8:08 [PATCH] rtw88: fix potential NULL skb access in TX ISR yhchuang
2020-01-07 10:40 ` Chris Chiu
@ 2020-01-26 15:42 ` Kalle Valo
1 sibling, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2020-01-26 15:42 UTC (permalink / raw)
To: yhchuang; +Cc: linux-wireless, briannorris, mikhail.v.gavrilov, rtereguloff
<yhchuang@realtek.com> wrote:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Sometimes the TX queue may be empty and we could possible
> dequeue a NULL pointer, crash the kernel. If the skb is NULL
> then there is nothing to do, just leave the ISR.
>
> And the TX queue should not be empty here, so print an error
> to see if there is anything wrong for DMA ring.
>
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Patch applied to wireless-drivers-next.git, thanks.
f4f84ff8377d rtw88: fix potential NULL skb access in TX ISR
--
https://patchwork.kernel.org/patch/11320567/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-26 15:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-07 8:08 [PATCH] rtw88: fix potential NULL skb access in TX ISR yhchuang
2020-01-07 10:40 ` Chris Chiu
2020-01-07 11:21 ` Tony Chuang
2020-01-09 10:26 ` Chris Chiu
2020-01-26 15:42 ` Kalle Valo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).