[1/3] usb: host: xhci: fix oops when removing hcd

[1/3] usb: host: xhci: fix oops when removing hcd

[Peter Chen]

The USB3 HCD structure (xhci->shared_hcd) may be NULL when the USB3
interrupt occurs, the typical use case is we plug out Type-C-to-A cable,
and there is a USB device at the port. The platform controller driver
(eg, dwc3) unregisters xhci platform driver, and remove hcd accordingly.
It is very easy to reproduce if there is a USB3 HUB on the port, and there
is no USB3 device on HUB's port, the controller is at U3, it performs
a far-end receiver termination detection every 100 ms, so the USB3 port
change interrupt may be occurs 100ms maximum later when we plug out
Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
The oops log like below:

[681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1
[681.787490] usb usb4: USB disconnect, device number 1
[681.792808] usb 4-1: USB disconnect, device number 2
[681.818089] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[681.823803] Unable to handle kernel NULL pointer dereference at virtual address 000000a0
[681.823806] Mem abort info:
[681.823809]   Exception class = DABT (current EL), IL = 32 bits
[681.823811]   SET = 0, FnV = 0
[681.823813]   EA = 0, S1PTW = 0
[681.823814] Data abort info:
[681.823816]   ISV = 0, ISS = 0x00000004
[681.823818]   CM = 0, WnR = 0
[681.823822] user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000ae3fd000
[681.823824] [00000000000000a0] *pgd=0000000000000000
[681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O) crct10dif_ce galcore(O)
[681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-imx_4.14.y+gcd63def #1
[681.823851] Hardware name: Freescale i.MX8MQ EVK (DT)
[681.823862] Workqueue: events_freezable __dwc3_set_mode
[681.823865] task: ffff8000b8a18000 task.stack: ffff00000a010000
[681.823872] PC is at xhci_irq+0x5fc/0x14b8
[681.823875] LR is at xhci_irq+0x3c/0x14b8
[681.823878] pc : [<ffff0000088ff204>] lr : [<ffff0000088fec44>] pstate: 800001c5
[681.823880] sp : ffff000008003b20
[681.823882] x29: ffff000008003b20 x28: ffff00000a53df10
[681.823886] x27: ffff8000ae3c0238 x26: 00000000000001c0
[681.823890] x25: ffff8000ae3c0000 x24: ffff00000a53df10
[681.823893] x23: 0000000000000000 x22: ffff000009297db8
[681.823897] x21: ffff000008f3f0e8 x20: 0000000000000001
[681.823901] x19: ffff8000ae3c0284 x18: 0000000000000010
[681.823904] x17: 0000000000427268 x16: 00000000000000f1
[681.823908] x15: ffffffffffffffff x14: 0000000000000000
[681.823911] x13: ffff000008db0500 x12: 0000000000000001
[681.823915] x11: ffff000008db0508 x10: 0000000000000040
[681.823918] x9 : ffff0000094e4088 x8 : ffff8000b6020000
[681.823922] x7 : ffff8000b6020028 x6 : 0000000000000001
[681.823925] x5 : 0000000000000001 x4 : 0000000000000000
[681.823928] x3 : 0000000000000003 x2 : 0000000000000002
[681.823932] x1 : ffff8000ae3c0000 x0 : ffff8000b48b8c00
[681.823936] Process kworker/0:1 (pid: 94, stack limit = 0xffff00000a010000)
[681.823938] Call trace:
[681.823942] Exception stack(0xffff0000080039e0 to 0xffff000008003b20)
[681.823946] 39e0: ffff8000b48b8c00 ffff8000ae3c0000 0000000000000002 0000000000000003
[681.823950] 3a00: 0000000000000000 0000000000000001 0000000000000001 ffff8000b6020028
[681.823953] 3a20: ffff8000b6020000 ffff0000094e4088 0000000000000040 ffff000008db0508
[681.823957] 3a40: 0000000000000001 ffff000008db0500 0000000000000000 ffffffffffffffff
[681.823960] 3a60: 00000000000000f1 0000000000427268 0000000000000010 ffff8000ae3c0284
[681.823964] 3a80: 0000000000000001 ffff000008f3f0e8 ffff000009297db8 0000000000000000
[681.823968] 3aa0: ffff00000a53df10 ffff8000ae3c0000 00000000000001c0 ffff8000ae3c0238
[681.823972] 3ac0: ffff00000a53df10 ffff000008003b20 ffff0000088fec44 ffff000008003b20
[681.823975] 3ae0: ffff0000088ff204 00000000800001c5 ffff7e0002d0bb40 ffff8000b42ed880
[681.823979] 3b00: 0000ffffffffffff ffff0000081fd2b4 ffff000008003b20 ffff0000088ff204
[681.823984] [<ffff0000088ff204>] xhci_irq+0x5fc/0x14b8
[681.823991] [<ffff00000889d3a4>] usb_hcd_irq+0x2c/0x48
[681.823997] [<ffff000008123ecc>] __handle_irq_event_percpu+0x5c/0x148
[681.824000] [<ffff000008123fd4>] handle_irq_event_percpu+0x1c/0x58
[681.824003] [<ffff000008124058>] handle_irq_event+0x48/0x78
[681.824008] [<ffff000008127e30>] handle_fasteoi_irq+0xa8/0x180
[681.824014] [<ffff000008122fec>] generic_handle_irq+0x24/0x38
[681.824018] [<ffff00000812366c>] __handle_domain_irq+0x5c/0xb8
[681.824023] [<ffff000008081960>] gic_handle_irq+0x78/0x17c
[681.824026] Exception stack(0xffff000008003d90 to 0xffff000008003ed0)
[681.824029] 3d80:                                   0000000000000000 ffff000009667700
[681.824032] 3da0: 0000000000000708 0000000000000000 0000000000000016 00ffffffffffffff
[681.824036] 3dc0: 00000000363b4285 7fffffffffffffff ffff8000bdf62db8 0000000000000004
[681.824039] 3de0: 00000000000000f1 ffff000008db0508 0000000000000001 ffff000008db0500
[681.824043] 3e00: 0000000000000000 ffffffffffffffff 00000000000000f1 0000000000427268
[681.824047] 3e20: 0000000000000010 ffff8000b8a18000 0000000000000003 ffff000009667700
[681.824051] 3e40: ffff0000094b0018 ffff8000b8006c00 ffff000009664000 00000001000174c1
[681.824055] 3e60: ffff000008004000 ffff000008daf000 0000000000000202 ffff000008003ed0
[681.824058] 3e80: ffff000008081ad4 ffff000008003ed0 ffff000008081b14 0000000040000145
[681.824062] 3ea0: ffff8000b8a18000 0000000000000003 0000ffffffffffff 0000000000000003
[681.824065] 3ec0: ffff000008003ed0 ffff000008081b14
[681.824068] [<ffff000008083230>] el1_irq+0xb0/0x124
[681.824072] [<ffff000008081b14>] __do_softirq+0xac/0x228
[681.824078] [<ffff0000080d4aac>] irq_exit+0xc4/0x100
[681.824083] [<ffff000008123670>] __handle_domain_irq+0x60/0xb8
[681.824086] [<ffff000008081960>] gic_handle_irq+0x78/0x17c
[681.824089] Exception stack(0xffff00000a013770 to 0xffff00000a0138b0)
[681.824091] 3760:                                   0000000000000020 ffff8000b8a18000
[681.824095] 3780: 00008000b4aa6000 ffffffffffffffff 0000000000000000 0000000000000000
[681.824099] 37a0: 000000000000000f 2034207375622042 7473696765726564 0000000000000006
[681.824102] 37c0: ffff00000a013820 ffff000008633128 ffff0000094c9df8 ffff00000966d45d
[681.824106] 37e0: ffff00008966d44f ffffffffffffffff 0000ffff9bf1f358 0000000000427268
[681.824109] 3800: 0000000000000010 0000000000000000 0000000000000140 ffff00000966c000
[681.824113] 3820: ffff00000966cef8 ffff0000094c9000 ffff00000966dcc8 ffff0000094b0018
[681.824117] 3840: 0000000000000040 0000000000000000 ffff00000966cec8 ffff00000a0138b0
[681.824120] 3860: ffff000008121194 ffff00000a0138b0 ffff000008121198 0000000040000145
[681.824124] 3880: ffff00000966c000 ffff00000966cef8 ffffffffffffffff ffff00000966dcc8
[681.824127] 38a0: ffff00000a0138b0 ffff000008121198
[681.824130] [<ffff000008083230>] el1_irq+0xb0/0x124
[681.824134] [<ffff000008121198>] console_unlock.part.8+0x208/0x418
[681.824138] [<ffff000008121628>] vprintk_emit+0x250/0x2e0
[681.824144] [<ffff0000086d3fac>] dev_vprintk_emit+0x104/0x220
[681.824148] [<ffff0000086d4110>] dev_printk_emit+0x48/0x50
[681.824152] [<ffff0000086d41a4>] __dev_printk+0x3c/0x78
[681.824155] [<ffff0000086d44cc>] _dev_info+0x4c/0x58
[681.824159] [<ffff00000889e53c>] usb_deregister_bus+0x34/0x70
[681.824162] [<ffff00000889f578>] usb_remove_hcd+0x130/0x1d8
[681.824165] [<ffff000008903640>] xhci_plat_remove+0x48/0xe0
[681.824170] [<ffff0000086da70c>] platform_drv_remove+0x24/0x50
[681.824175] [<ffff0000086d8e44>] device_release_driver_internal+0x174/0x208
[681.824180] [<ffff0000086d8eec>] device_release_driver+0x14/0x20
[681.824184] [<ffff0000086d7c8c>] bus_remove_device+0x114/0x128
[681.824188] [<ffff0000086d480c>] device_del+0x1ac/0x300
[681.824192] [<ffff0000086dad1c>] platform_device_del.part.3+0x24/0x90
[681.824196] [<ffff0000086dadc0>] platform_device_unregister+0x18/0x38
[681.824200] [<ffff0000088b8b8c>] dwc3_host_exit+0x54/0x68
[681.824204] [<ffff0000088b6e5c>] __dwc3_set_mode+0x1f4/0x210
[681.824210] [<ffff0000080e8aec>] process_one_work+0x1d4/0x348
[681.824213] [<ffff0000080e8ca8>] worker_thread+0x48/0x470
[681.824218] [<ffff0000080eefac>] kthread+0x12c/0x130
[681.824223] [<ffff000008084ed8>] ret_from_fork+0x10/0x18
[681.824227] Code: f9400777 34002183 7103fc7f 54003680 (b940a2e4)
[681.824236] ---[ end trace 3ecbb9c7f9dc3d7f ]---
[681.824239] Kernel panic - not syncing: Fatal exception in interrupt
[681.824243] SMP: stopping secondary CPUs
[681.824298] Kernel Offset: disabled
[681.824301] CPU features: 0x080200c
[681.824303] Memory Limit: none
[682.518113] ---[ end Kernel panic - not syncing: Fatal exception in interrupt

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-ring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f0a99aa0ac58..2dc5176b79d0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	}
 
 	if (xhci->xhc_state & XHCI_STATE_DYING ||
-	    xhci->xhc_state & XHCI_STATE_HALTED) {
+	    xhci->xhc_state & XHCI_STATE_HALTED ||
+	    xhci->xhc_state & XHCI_STATE_REMOVING) {
 		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
 				"Shouldn't IRQs be disabled?\n");
 		/* Clear the event handler busy flag (RW1C);

[1/3] usb: host: xhci: fix oops when removing hcd

[Jack Pham]

On Wed, Sep 26, 2018 at 06:34:30PM -0700, Jack Pham wrote:
> Hi Mathias,
> 
> On Wed, Sep 26, 2018 at 02:51:40PM +0300, Mathias Nyman wrote:
> > Hi Jack, Peter
> > 
> > On 24.09.2018 19:37, Jack Pham wrote:
> > >Hi Peter,
> > >
> > >On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
> > >>Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
> > >>The oops log like below:
> > >>
> > >>[681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1
> > >>[681.787490] usb usb4: USB disconnect, device number 1
> > >>[681.792808] usb 4-1: USB disconnect, device number 2
> > >>[681.818089] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
> > >>[681.823803] Unable to handle kernel NULL pointer dereference at virtual address 000000a0
> > >>[681.823806] Mem abort info:
> > >>[681.823809]   Exception class = DABT (current EL), IL = 32 bits
> > >>[681.823811]   SET = 0, FnV = 0
> > >>[681.823813]   EA = 0, S1PTW = 0
> > >>[681.823814] Data abort info:
> > >>[681.823816]   ISV = 0, ISS = 0x00000004
> > >>[681.823818]   CM = 0, WnR = 0
> > >>[681.823822] user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000ae3fd000
> > >>[681.823824] [00000000000000a0] *pgd=0000000000000000
> > >>[681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > >>[681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O) crct10dif_ce galcore(O)
> > >>[681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-imx_4.14.y+gcd63def #1
> > >>[681.823851] Hardware name: Freescale i.MX8MQ EVK (DT)
> > >>[681.823862] Workqueue: events_freezable __dwc3_set_mode
> > >>[681.823865] task: ffff8000b8a18000 task.stack: ffff00000a010000
> > >>[681.823872] PC is at xhci_irq+0x5fc/0x14b8
> > >>[681.823875] LR is at xhci_irq+0x3c/0x14b8
> > >
> > ><snip>
> > >
> > >>diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > >>index f0a99aa0ac58..2dc5176b79d0 100644
> > >>--- a/drivers/usb/host/xhci-ring.c
> > >>+++ b/drivers/usb/host/xhci-ring.c
> > >>@@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
> > >>  	}
> > >>  	if (xhci->xhc_state & XHCI_STATE_DYING ||
> > >>-	    xhci->xhc_state & XHCI_STATE_HALTED) {
> > >>+	    xhci->xhc_state & XHCI_STATE_HALTED ||
> > >>+	    xhci->xhc_state & XHCI_STATE_REMOVING) {
> > >>  		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
> > >>  				"Shouldn't IRQs be disabled?\n");
> > >>  		/* Clear the event handler busy flag (RW1C);
> > >
> > >We also noticed the same crash as you found, and tried to fix it in a
> > >similar way, but noticed that this still allows a memory leak to happen.
> > >
> > >It seems from commit fe190ed0d602a ("xhci: Do not halt the host until
> > >both HCD have disconnected their devices.") this was added to
> > >xhci_stop(), and is the reason we encounter the NULL pointer in
> > >xhci_irq() when it tries to access xhci->shared_hcd.
> > >
> > >	+       /* Only halt host and free memory after both hcds are removed */
> > >		if (!usb_hcd_is_primary_hcd(hcd)) {
> > >	+               /* usb core will free this hcd shortly, unset pointer */
> > >	+               xhci->shared_hcd = NULL;
> > >			mutex_unlock(&xhci->mutex);
> > >			return;
> > >		}
> > >
> > >While your fix will simply abort the xhci_irq() function if it
> > >encounters XHCI_STATE_REMOVING, during xhci_plat_remove():
> > >
> > >	static int xhci_plat_remove(struct platform_device *dev)
> > >	{
> > >		struct usb_hcd  *hcd = platform_get_drvdata(dev);
> > >		struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > >		struct clk *clk = xhci->clk;
> > >		struct clk *reg_clk = xhci->reg_clk;
> > >
> > >		xhci->xhc_state |= XHCI_STATE_REMOVING;
> > >
> > >		usb_remove_hcd(xhci->shared_hcd);
> > >		^^^^^^^^^ calls xhci_stop() and sets shared_hcd=NULL
> > >		usb_phy_shutdown(hcd->usb_phy);
> > >
> > >		usb_remove_hcd(hcd);
> > >		usb_put_hcd(xhci->shared_hcd);
> > >		^^^^^^^^^^^ shared_hcd==NULL, so this is a no-op
> > >
> > >Since usb_put_hcd() doesn't get called for shared_hcd, we end up
> > >with one additional kref count and hence a leak.
> > 
> > Nice catch,  this same issue exists in
> > 
> > xhci_pci_remove()
> > tegra_xusb_remove()
> > xhci_mtk_remove()
> > xhci_histb_remove()
> > 
> > 
> > >
> > >Wondering if we need to also remove the xhci->shared_hcd = NULL from
> > >xhci_stop(), in addition to your patches. Thoughts?
> > 
> > At some point the xhci->shared_hcd needs to be set NULL, it can be done in
> > in the xhci_plat_remove(), xhci_pci_remove() and the similar remove functions
> > after calling usb_remove_hcd(). we can't rely on xhci->shared_hcd after it
> > has been removed.
> 
> Currently it is already done as follows:
> 
> usb_remove_hcd(xhci->shared_hcd)
>   -> xhci_stop()
>         /* Only halt host and free memory after both hcds are removed */
>         if (!usb_hcd_is_primary_hcd(hcd)) {
>                 /* usb core will free this hcd shortly, unset pointer */
>                 xhci->shared_hcd = NULL;
> 
> I guess at the very least your suggestion to do it from the
> xhci_{plat,pci,mtk,tegra}_remove() context makes it more explicit rather
> than having it as a side effect of usb_remove_hcd()/xhci_stop(). The net
> effect is more or less the same though IMO.

D'oh. Of course it makes sense to move the NULL assignment to the caller
*after* calling usb_put_hcd() so it can be properly freed.

Jack

[1/3] usb: host: xhci: fix oops when removing hcd

[Jack Pham]

Hi Mathias,

On Wed, Sep 26, 2018 at 02:51:40PM +0300, Mathias Nyman wrote:
> Hi Jack, Peter
> 
> On 24.09.2018 19:37, Jack Pham wrote:
> >Hi Peter,
> >
> >On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
> >>Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
> >>The oops log like below:
> >>
> >>[681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1
> >>[681.787490] usb usb4: USB disconnect, device number 1
> >>[681.792808] usb 4-1: USB disconnect, device number 2
> >>[681.818089] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
> >>[681.823803] Unable to handle kernel NULL pointer dereference at virtual address 000000a0
> >>[681.823806] Mem abort info:
> >>[681.823809]   Exception class = DABT (current EL), IL = 32 bits
> >>[681.823811]   SET = 0, FnV = 0
> >>[681.823813]   EA = 0, S1PTW = 0
> >>[681.823814] Data abort info:
> >>[681.823816]   ISV = 0, ISS = 0x00000004
> >>[681.823818]   CM = 0, WnR = 0
> >>[681.823822] user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000ae3fd000
> >>[681.823824] [00000000000000a0] *pgd=0000000000000000
> >>[681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> >>[681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O) crct10dif_ce galcore(O)
> >>[681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-imx_4.14.y+gcd63def #1
> >>[681.823851] Hardware name: Freescale i.MX8MQ EVK (DT)
> >>[681.823862] Workqueue: events_freezable __dwc3_set_mode
> >>[681.823865] task: ffff8000b8a18000 task.stack: ffff00000a010000
> >>[681.823872] PC is at xhci_irq+0x5fc/0x14b8
> >>[681.823875] LR is at xhci_irq+0x3c/0x14b8
> >
> ><snip>
> >
> >>diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> >>index f0a99aa0ac58..2dc5176b79d0 100644
> >>--- a/drivers/usb/host/xhci-ring.c
> >>+++ b/drivers/usb/host/xhci-ring.c
> >>@@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
> >>  	}
> >>  	if (xhci->xhc_state & XHCI_STATE_DYING ||
> >>-	    xhci->xhc_state & XHCI_STATE_HALTED) {
> >>+	    xhci->xhc_state & XHCI_STATE_HALTED ||
> >>+	    xhci->xhc_state & XHCI_STATE_REMOVING) {
> >>  		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
> >>  				"Shouldn't IRQs be disabled?\n");
> >>  		/* Clear the event handler busy flag (RW1C);
> >
> >We also noticed the same crash as you found, and tried to fix it in a
> >similar way, but noticed that this still allows a memory leak to happen.
> >
> >It seems from commit fe190ed0d602a ("xhci: Do not halt the host until
> >both HCD have disconnected their devices.") this was added to
> >xhci_stop(), and is the reason we encounter the NULL pointer in
> >xhci_irq() when it tries to access xhci->shared_hcd.
> >
> >	+       /* Only halt host and free memory after both hcds are removed */
> >		if (!usb_hcd_is_primary_hcd(hcd)) {
> >	+               /* usb core will free this hcd shortly, unset pointer */
> >	+               xhci->shared_hcd = NULL;
> >			mutex_unlock(&xhci->mutex);
> >			return;
> >		}
> >
> >While your fix will simply abort the xhci_irq() function if it
> >encounters XHCI_STATE_REMOVING, during xhci_plat_remove():
> >
> >	static int xhci_plat_remove(struct platform_device *dev)
> >	{
> >		struct usb_hcd  *hcd = platform_get_drvdata(dev);
> >		struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >		struct clk *clk = xhci->clk;
> >		struct clk *reg_clk = xhci->reg_clk;
> >
> >		xhci->xhc_state |= XHCI_STATE_REMOVING;
> >
> >		usb_remove_hcd(xhci->shared_hcd);
> >		^^^^^^^^^ calls xhci_stop() and sets shared_hcd=NULL
> >		usb_phy_shutdown(hcd->usb_phy);
> >
> >		usb_remove_hcd(hcd);
> >		usb_put_hcd(xhci->shared_hcd);
> >		^^^^^^^^^^^ shared_hcd==NULL, so this is a no-op
> >
> >Since usb_put_hcd() doesn't get called for shared_hcd, we end up
> >with one additional kref count and hence a leak.
> 
> Nice catch,  this same issue exists in
> 
> xhci_pci_remove()
> tegra_xusb_remove()
> xhci_mtk_remove()
> xhci_histb_remove()
> 
> 
> >
> >Wondering if we need to also remove the xhci->shared_hcd = NULL from
> >xhci_stop(), in addition to your patches. Thoughts?
> 
> At some point the xhci->shared_hcd needs to be set NULL, it can be done in
> in the xhci_plat_remove(), xhci_pci_remove() and the similar remove functions
> after calling usb_remove_hcd(). we can't rely on xhci->shared_hcd after it
> has been removed.

Currently it is already done as follows:

usb_remove_hcd(xhci->shared_hcd)
  -> xhci_stop()
        /* Only halt host and free memory after both hcds are removed */
        if (!usb_hcd_is_primary_hcd(hcd)) {
                /* usb core will free this hcd shortly, unset pointer */
                xhci->shared_hcd = NULL;

I guess at the very least your suggestion to do it from the
xhci_{plat,pci,mtk,tegra}_remove() context makes it more explicit rather
than having it as a side effect of usb_remove_hcd()/xhci_stop(). The net
effect is more or less the same though IMO.

> The xHC controller is stopped and interrupts disabled only after the primary
> hcd (USB2) is removed, this is because usb core will try to cleanly flush last USB2
> URBs and take down the last endpoints when the USB2 usb_remove_hcd(hcd) is called.
> We need a running xHC controller to do this.
> 
> This means we can get interrupts we need to handle even if the shared_hcd is removed.
> So I think we still need to handle interrupts even if XHCI_STATE_REMOVING flag is set.
> 
> I think we also need to make the xhci interrupt handler capable of handling situations
> where xhci->shared_hcd is set to NULL
> 
> Does this sound reasonable?

I see your point. So Peter's patchset might have unintended consequences
by preventing further interrupts to be handled when there is some
necessary cleanup to be done?

I realize now that Peter and I are both encountering the NULL pointer on
4.14. I believe it's when xhci_irq() calls to handle_port_status():

        /* Figure out which usb_hcd this port is attached to:
         * is it a USB 3.0 port or a USB 2.0/1.1 port?
         */
        major_revision = xhci->port_array[port_id - 1];

        /* Find the right roothub. */
        hcd = xhci_to_hcd(xhci);
        if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
                hcd = xhci->shared_hcd;

then later tries to dereference hcd which was taken from the NULL
shared_hcd:

        bus_state = &xhci->bus_state[hcd_index(hcd)];
        if (hcd->speed >= HCD_USB3)
                port_array = xhci->usb3_ports;
        else
                port_array = xhci->usb2_ports;

Since 4.18 you had recently added commit 52c7755ba19e ("xhci: xhci-ring:
use port structures for port event handler") which replaces the above
by deriving hcd from the xhci->hw_ports array instead of assigning it
from shared_hcd. So I guess this issue probably won't be encountered on
mainline (sorry, I have yet to test this). Since the problem is there
from 4.12 to 4.17 due to commit fe190ed0d602, do you have a suggestion
on how to fix it for -stable?

Thanks,
Jack

[1/3] usb: host: xhci: fix oops when removing hcd

[Mathias Nyman]

Hi Jack, Peter

On 24.09.2018 19:37, Jack Pham wrote:
> Hi Peter,
> 
> On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
>> Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
>> The oops log like below:
>>
>> [681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1
>> [681.787490] usb usb4: USB disconnect, device number 1
>> [681.792808] usb 4-1: USB disconnect, device number 2
>> [681.818089] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
>> [681.823803] Unable to handle kernel NULL pointer dereference at virtual address 000000a0
>> [681.823806] Mem abort info:
>> [681.823809]   Exception class = DABT (current EL), IL = 32 bits
>> [681.823811]   SET = 0, FnV = 0
>> [681.823813]   EA = 0, S1PTW = 0
>> [681.823814] Data abort info:
>> [681.823816]   ISV = 0, ISS = 0x00000004
>> [681.823818]   CM = 0, WnR = 0
>> [681.823822] user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000ae3fd000
>> [681.823824] [00000000000000a0] *pgd=0000000000000000
>> [681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>> [681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O) crct10dif_ce galcore(O)
>> [681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-imx_4.14.y+gcd63def #1
>> [681.823851] Hardware name: Freescale i.MX8MQ EVK (DT)
>> [681.823862] Workqueue: events_freezable __dwc3_set_mode
>> [681.823865] task: ffff8000b8a18000 task.stack: ffff00000a010000
>> [681.823872] PC is at xhci_irq+0x5fc/0x14b8
>> [681.823875] LR is at xhci_irq+0x3c/0x14b8
> 
> <snip>
> 
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index f0a99aa0ac58..2dc5176b79d0 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>>   	}
>>   
>>   	if (xhci->xhc_state & XHCI_STATE_DYING ||
>> -	    xhci->xhc_state & XHCI_STATE_HALTED) {
>> +	    xhci->xhc_state & XHCI_STATE_HALTED ||
>> +	    xhci->xhc_state & XHCI_STATE_REMOVING) {
>>   		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
>>   				"Shouldn't IRQs be disabled?\n");
>>   		/* Clear the event handler busy flag (RW1C);
> 
> We also noticed the same crash as you found, and tried to fix it in a
> similar way, but noticed that this still allows a memory leak to happen.
> 
> It seems from commit fe190ed0d602a ("xhci: Do not halt the host until
> both HCD have disconnected their devices.") this was added to
> xhci_stop(), and is the reason we encounter the NULL pointer in
> xhci_irq() when it tries to access xhci->shared_hcd.
> 
> 	+       /* Only halt host and free memory after both hcds are removed */
> 		if (!usb_hcd_is_primary_hcd(hcd)) {
> 	+               /* usb core will free this hcd shortly, unset pointer */
> 	+               xhci->shared_hcd = NULL;
> 			mutex_unlock(&xhci->mutex);
> 			return;
> 		}
> 
> While your fix will simply abort the xhci_irq() function if it
> encounters XHCI_STATE_REMOVING, during xhci_plat_remove():
> 
> 	static int xhci_plat_remove(struct platform_device *dev)
> 	{
> 		struct usb_hcd  *hcd = platform_get_drvdata(dev);
> 		struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> 		struct clk *clk = xhci->clk;
> 		struct clk *reg_clk = xhci->reg_clk;
> 
> 		xhci->xhc_state |= XHCI_STATE_REMOVING;
> 
> 		usb_remove_hcd(xhci->shared_hcd);
> 		^^^^^^^^^ calls xhci_stop() and sets shared_hcd=NULL
> 		usb_phy_shutdown(hcd->usb_phy);
> 
> 		usb_remove_hcd(hcd);
> 		usb_put_hcd(xhci->shared_hcd);
> 		^^^^^^^^^^^ shared_hcd==NULL, so this is a no-op
> 
> Since usb_put_hcd() doesn't get called for shared_hcd, we end up
> with one additional kref count and hence a leak.

Nice catch,  this same issue exists in

xhci_pci_remove()
tegra_xusb_remove()
xhci_mtk_remove()
xhci_histb_remove()


> 
> Wondering if we need to also remove the xhci->shared_hcd = NULL from
> xhci_stop(), in addition to your patches. Thoughts?

At some point the xhci->shared_hcd needs to be set NULL, it can be done in
in the xhci_plat_remove(), xhci_pci_remove() and the similar remove functions
after calling usb_remove_hcd(). we can't rely on xhci->shared_hcd after it
has been removed.

The xHC controller is stopped and interrupts disabled only after the primary
hcd (USB2) is removed, this is because usb core will try to cleanly flush last USB2
URBs and take down the last endpoints when the USB2 usb_remove_hcd(hcd) is called.
We need a running xHC controller to do this.

This means we can get interrupts we need to handle even if the shared_hcd is removed.
So I think we still need to handle interrupts even if XHCI_STATE_REMOVING flag is set.

I think we also need to make the xhci interrupt handler capable of handling situations
where xhci->shared_hcd is set to NULL

Does this sound reasonable?

-Mathias

[1/3] usb: host: xhci: fix oops when removing hcd

[Peter Chen]

> Hi Peter,
> 
> On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
> > Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
> > The oops log like below:
> >
> > [681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1 [681.787490]
> > usb usb4: USB disconnect, device number 1 [681.792808] usb 4-1: USB
> > disconnect, device number 2 [681.818089] xhci-hcd xhci-hcd.1.auto: USB
> > bus 4 deregistered [681.823803] Unable to handle kernel NULL pointer
> > dereference at virtual address 000000a0 [681.823806] Mem abort info:
> > [681.823809]   Exception class = DABT (current EL), IL = 32 bits
> > [681.823811]   SET = 0, FnV = 0
> > [681.823813]   EA = 0, S1PTW = 0
> > [681.823814] Data abort info:
> > [681.823816]   ISV = 0, ISS = 0x00000004
> > [681.823818]   CM = 0, WnR = 0
> > [681.823822] user pgtable: 4k pages, 48-bit VAs, pgd =
> > ffff8000ae3fd000 [681.823824] [00000000000000a0] *pgd=0000000000000000
> > [681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > [681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O)
> crct10dif_ce galcore(O)
> > [681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-
> imx_4.14.y+gcd63def #1
> > [681.823851] Hardware name: Freescale i.MX8MQ EVK (DT) [681.823862]
> > Workqueue: events_freezable __dwc3_set_mode [681.823865] task:
> > ffff8000b8a18000 task.stack: ffff00000a010000 [681.823872] PC is at
> > xhci_irq+0x5fc/0x14b8 [681.823875] LR is at xhci_irq+0x3c/0x14b8
> 
> <snip>
> 
> > diff --git a/drivers/usb/host/xhci-ring.c
> > b/drivers/usb/host/xhci-ring.c index f0a99aa0ac58..2dc5176b79d0 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
> >  	}
> >
> >  	if (xhci->xhc_state & XHCI_STATE_DYING ||
> > -	    xhci->xhc_state & XHCI_STATE_HALTED) {
> > +	    xhci->xhc_state & XHCI_STATE_HALTED ||
> > +	    xhci->xhc_state & XHCI_STATE_REMOVING) {
> >  		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
> >  				"Shouldn't IRQs be disabled?\n");
> >  		/* Clear the event handler busy flag (RW1C);
> 
> We also noticed the same crash as you found, and tried to fix it in a similar way, but
> noticed that this still allows a memory leak to happen.
> 
> It seems from commit fe190ed0d602a ("xhci: Do not halt the host until both HCD
> have disconnected their devices.") this was added to xhci_stop(), and is the reason
> we encounter the NULL pointer in
> xhci_irq() when it tries to access xhci->shared_hcd.
> 
> 	+       /* Only halt host and free memory after both hcds are removed */
> 		if (!usb_hcd_is_primary_hcd(hcd)) {
> 	+               /* usb core will free this hcd shortly, unset pointer */
> 	+               xhci->shared_hcd = NULL;
> 			mutex_unlock(&xhci->mutex);
> 			return;
> 		}
> 
> While your fix will simply abort the xhci_irq() function if it encounters
> XHCI_STATE_REMOVING, during xhci_plat_remove():
> 
> 	static int xhci_plat_remove(struct platform_device *dev)
> 	{
> 		struct usb_hcd  *hcd = platform_get_drvdata(dev);
> 		struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> 		struct clk *clk = xhci->clk;
> 		struct clk *reg_clk = xhci->reg_clk;
> 
> 		xhci->xhc_state |= XHCI_STATE_REMOVING;
> 
> 		usb_remove_hcd(xhci->shared_hcd);
> 		^^^^^^^^^ calls xhci_stop() and sets shared_hcd=NULL
> 		usb_phy_shutdown(hcd->usb_phy);
> 
> 		usb_remove_hcd(hcd);
> 		usb_put_hcd(xhci->shared_hcd);
> 		^^^^^^^^^^^ shared_hcd==NULL, so this is a no-op
> 
> Since usb_put_hcd() doesn't get called for shared_hcd, we end up with one
> additional kref count and hence a leak.
> 
> Wondering if we need to also remove the xhci->shared_hcd = NULL from xhci_stop(),
> in addition to your patches. Thoughts?
> 
 
Hi Jack,

With your thought of removing the xhci->shared_hcd = NULL at xhci_stop, the oops in
this commit mentioned has disappeared. It seems removing USB3 HCD structure
not affect it handles USB3 interrupt during the disconnection. Please have a test if
only this change can fix your problem. You could submit a patch for this change
as a fix, it fixed the memory leak for USB3 HCD structure too.

Peter

[1/3] usb: host: xhci: fix oops when removing hcd

[Jack Pham]

Hi Peter,

On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
> Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
> The oops log like below:
> 
> [681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1
> [681.787490] usb usb4: USB disconnect, device number 1
> [681.792808] usb 4-1: USB disconnect, device number 2
> [681.818089] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
> [681.823803] Unable to handle kernel NULL pointer dereference at virtual address 000000a0
> [681.823806] Mem abort info:
> [681.823809]   Exception class = DABT (current EL), IL = 32 bits
> [681.823811]   SET = 0, FnV = 0
> [681.823813]   EA = 0, S1PTW = 0
> [681.823814] Data abort info:
> [681.823816]   ISV = 0, ISS = 0x00000004
> [681.823818]   CM = 0, WnR = 0
> [681.823822] user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000ae3fd000
> [681.823824] [00000000000000a0] *pgd=0000000000000000
> [681.823829] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O) crct10dif_ce galcore(O)
> [681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G           O    4.14.62-imx_4.14.y+gcd63def #1
> [681.823851] Hardware name: Freescale i.MX8MQ EVK (DT)
> [681.823862] Workqueue: events_freezable __dwc3_set_mode
> [681.823865] task: ffff8000b8a18000 task.stack: ffff00000a010000
> [681.823872] PC is at xhci_irq+0x5fc/0x14b8
> [681.823875] LR is at xhci_irq+0x3c/0x14b8

<snip>

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index f0a99aa0ac58..2dc5176b79d0 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>  	}
>  
>  	if (xhci->xhc_state & XHCI_STATE_DYING ||
> -	    xhci->xhc_state & XHCI_STATE_HALTED) {
> +	    xhci->xhc_state & XHCI_STATE_HALTED ||
> +	    xhci->xhc_state & XHCI_STATE_REMOVING) {
>  		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
>  				"Shouldn't IRQs be disabled?\n");
>  		/* Clear the event handler busy flag (RW1C);

We also noticed the same crash as you found, and tried to fix it in a
similar way, but noticed that this still allows a memory leak to happen.

It seems from commit fe190ed0d602a ("xhci: Do not halt the host until
both HCD have disconnected their devices.") this was added to
xhci_stop(), and is the reason we encounter the NULL pointer in
xhci_irq() when it tries to access xhci->shared_hcd.

	+       /* Only halt host and free memory after both hcds are removed */
		if (!usb_hcd_is_primary_hcd(hcd)) {
	+               /* usb core will free this hcd shortly, unset pointer */
	+               xhci->shared_hcd = NULL;
			mutex_unlock(&xhci->mutex);
			return;
		}

While your fix will simply abort the xhci_irq() function if it
encounters XHCI_STATE_REMOVING, during xhci_plat_remove():

	static int xhci_plat_remove(struct platform_device *dev)
	{
		struct usb_hcd  *hcd = platform_get_drvdata(dev);
		struct xhci_hcd *xhci = hcd_to_xhci(hcd);
		struct clk *clk = xhci->clk;
		struct clk *reg_clk = xhci->reg_clk;

		xhci->xhc_state |= XHCI_STATE_REMOVING;

		usb_remove_hcd(xhci->shared_hcd);
		^^^^^^^^^ calls xhci_stop() and sets shared_hcd=NULL
		usb_phy_shutdown(hcd->usb_phy);

		usb_remove_hcd(hcd);
		usb_put_hcd(xhci->shared_hcd);
		^^^^^^^^^^^ shared_hcd==NULL, so this is a no-op

Since usb_put_hcd() doesn't get called for shared_hcd, we end up
with one additional kref count and hence a leak.

Wondering if we need to also remove the xhci->shared_hcd = NULL from
xhci_stop(), in addition to your patches. Thoughts?

Thanks,
Jack