Re: [PATCH] mt7601u: fix possible memory leak when the device is disconnected

[Jakub Kicinski <kubakici@wp.pl>]

On Thu, 6 Jun 2019 23:10:15 +0200, Lorenzo Bianconi wrote:
> On Thu, Jun 6, 2019 at 8:49 PM Jakub Kicinski <kubakici@wp.pl> wrote:
> >
> > On Thu,  6 Jun 2019 17:34:16 +0200, Lorenzo Bianconi wrote:  
> > > When the device is disconnected while passing traffic it is possible
> > > to receive out of order urbs causing a memory leak since the skb liked
> > > to the current tx urb is not removed. Fix the issue deallocating the skb
> > > cleaning up the tx ring. Moreover this patch fixes the following kernel
> > > warning  
> >
> > Ugh if we don't have ordering guarantees then the entire "ring" scheme
> > no longer works :(  Should we move to URB queues, I don't remember now,
> > but there seem to had been a better way to manage URBs :S
> >  
> 
> actually I have observed these issues on tx/rx side just during device
> disconnection while passing traffic.
> I guess we can assume a proper urb ordering during normal operation
> (and so tx/rx ring works fine).

Ah, phew, okay.  That's fine then.

> Btw what do you mean with 'URB queues'? :)

I think it may have been URB anchors.  I remember looking at carl9170
and liking how the DMAs operated there.  

> > > [   57.480771] usb 1-1: USB disconnect, device number 2
> > > [   57.483451] ------------[ cut here ]------------
> > > [   57.483462] TX urb mismatch
> > > [   57.483481] WARNING: CPU: 1 PID: 32 at drivers/net/wireless/mediatek/mt7601u/dma.c:245 mt7601u_complete_tx+0x165/00
> > > [   57.483483] Modules linked in:
> > > [   57.483496] CPU: 1 PID: 32 Comm: kworker/1:1 Not tainted 5.2.0-rc1+ #72
> > > [   57.483498] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.12.0-2.fc30 04/01/2014
> > > [   57.483502] Workqueue: usb_hub_wq hub_event
> > > [   57.483507] RIP: 0010:mt7601u_complete_tx+0x165/0x1e0
> > > [   57.483510] Code: 8b b5 10 04 00 00 8b 8d 14 04 00 00 eb 8b 80 3d b1 cb e1 00 00 75 9e 48 c7 c7 a4 ea 05 82 c6 05 f
> > > [   57.483513] RSP: 0000:ffffc900000a0d28 EFLAGS: 00010092
> > > [   57.483516] RAX: 000000000000000f RBX: ffff88802c0a62c0 RCX: ffffc900000a0c2c
> > > [   57.483518] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff810a8371
> > > [   57.483520] RBP: ffff88803ced6858 R08: 0000000000000000 R09: 0000000000000001
> > > [   57.483540] R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000046
> > > [   57.483542] R13: ffff88802c0a6c88 R14: ffff88803baab540 R15: ffff88803a0cc078
> > > [   57.483548] FS:  0000000000000000(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> > > [   57.483550] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   57.483552] CR2: 000055e7f6780100 CR3: 0000000028c86000 CR4: 00000000000006a0
> > > [   57.483554] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > [   57.483556] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > [   57.483559] Call Trace:
> > > [   57.483561]  <IRQ>
> > > [   57.483565]  __usb_hcd_giveback_urb+0x77/0xe0
> > > [   57.483570]  xhci_giveback_urb_in_irq.isra.0+0x8b/0x140
> > > [   57.483574]  handle_cmd_completion+0xf5b/0x12c0
> > > [   57.483577]  xhci_irq+0x1f6/0x1810
> > > [   57.483581]  ? lockdep_hardirqs_on+0x9e/0x180
> > > [   57.483584]  ? _raw_spin_unlock_irq+0x24/0x30
> > > [   57.483588]  __handle_irq_event_percpu+0x3a/0x260
> > > [   57.483592]  handle_irq_event_percpu+0x1c/0x60
> > > [   57.483595]  handle_irq_event+0x2f/0x4c
> > > [   57.483599]  handle_edge_irq+0x7e/0x1a0
> > > [   57.483603]  handle_irq+0x17/0x20
> > > [   57.483607]  do_IRQ+0x54/0x110
> > > [   57.483610]  common_interrupt+0xf/0xf
> > > [   57.483612]  </IRQ>
> > >
> > > Fixes: c869f77d6abb ("add mt7601u driver")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/wireless/mediatek/mt7601u/dma.c | 24 +++++++++++++++------
> > >  drivers/net/wireless/mediatek/mt7601u/tx.c  |  4 ++--
> > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > index e7703990b291..bbf1deed7f3b 100644
> > > --- a/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > +++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > @@ -238,14 +238,25 @@ static void mt7601u_complete_tx(struct urb *urb)
> > >       struct sk_buff *skb;
> > >       unsigned long flags;
> > >
> > > -     spin_lock_irqsave(&dev->tx_lock, flags);
> > > +     switch (urb->status) {
> > > +     case -ECONNRESET:
> > > +     case -ESHUTDOWN:
> > > +     case -ENOENT:
> > > +             return;
> > > +     default:
> > > +             dev_err_ratelimited(dev->dev, "tx urb failed: %d\n",
> > > +                                 urb->status);
> > > +             /* fall through */
> > > +     case 0:
> > > +             break;
> > > +     }
> > >
> > > -     if (mt7601u_urb_has_error(urb))
> > > -             dev_err(dev->dev, "Error: TX urb failed:%d\n", urb->status);
> > > +     spin_lock_irqsave(&dev->tx_lock, flags);
> > >       if (WARN_ONCE(q->e[q->start].urb != urb, "TX urb mismatch"))
> > >               goto out;
> > >
> > >       skb = q->e[q->start].skb;
> > > +     q->e[q->start].skb = NULL;
> > >       trace_mt_tx_dma_done(dev, skb);
> > >
> > >       __skb_queue_tail(&dev->tx_skb_done, skb);
> > > @@ -446,10 +457,10 @@ static void mt7601u_free_tx_queue(struct mt7601u_tx_queue *q)
> > >  {
> > >       int i;
> > >
> > > -     WARN_ON(q->used);
> > > -
> > >       for (i = 0; i < q->entries; i++)  {
> > >               usb_poison_urb(q->e[i].urb);
> > > +             if (q->e[i].skb)
> > > +                     mt7601u_tx_status(q->dev, q->e[i].skb);  
> >
> > Perhaps a separate patch?
> 
> As I did for rx side, if we do not schedule the tx tasklet when the
> device has been disconnected I guess we need this chuck in the same
> patch. What do you think?

Yeah, I see.