| Message ID | 20200107080807.14433-1-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4f84ff8377d4cedf18317747bc407b2cf657d0f |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: fix potential NULL skb access in TX ISR | expand |
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
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
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
<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
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);