Re: [BUG] usb: dwc3: Kernel NULL pointer dereference in dwc3_remove()

From: Felipe Balbi <balbi@kernel.org>
To: Jack Pham <jackp@codeaurora.org>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	p.zabel@pengutronix.de, linux-usb@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	arm-mail-list <linux-arm-kernel@lists.infradead.org>,
	sanm@codeaurora.org
Subject: Re: [BUG] usb: dwc3: Kernel NULL pointer dereference in dwc3_remove()
Date: Fri, 04 Jun 2021 11:20:12 +0300	[thread overview]
Message-ID: <87mts6avnn.fsf@kernel.org> (raw)
In-Reply-To: <20210603173632.GA25299@jackp-linux.qualcomm.com>

[-- Attachment #1: Type: text/plain, Size: 7528 bytes --]

Hi,

Jack Pham <jackp@codeaurora.org> writes:
>> >>>> Alexandru Elisei <alexandru.elisei@arm.com> writes:
>> >>>>> I've been seeing the following panic when shutting down my rockpro64:
>> >>>>>
>> >>>>> [ï¿½ï¿½ 21.459064] xhci-hcd xhci-hcd.0.auto: USB bus 5 deregistered
>> >>>>> [ï¿½ï¿½ 21.683077] Unable to handle kernel NULL pointer dereference at virtual address
>> >>>>> 00000000000000a0
>> >>>>> [ï¿½ï¿½ 21.683858] Mem abort info:
>> >>>>> [ï¿½ï¿½ 21.684104]ï¿½ï¿½ ESR = 0x96000004
>> >>>>> [ï¿½ï¿½ 21.684375]ï¿½ï¿½ EC = 0x25: DABT (current EL), IL = 32 bits
>> >>>>> [ï¿½ï¿½ 21.684841]ï¿½ï¿½ SET = 0, FnV = 0
>> >>>>> [ï¿½ï¿½ 21.685111]ï¿½ï¿½ EA = 0, S1PTW = 0
>> >>>>> [ï¿½ï¿½ 21.685389] Data abort info:
>> >>>>> [ï¿½ï¿½ 21.685644]ï¿½ï¿½ ISV = 0, ISS = 0x00000004
>> >>>>> [ï¿½ï¿½ 21.686024]ï¿½ï¿½ CM = 0, WnR = 0
>> >>>>> [ï¿½ï¿½ 21.686288] user pgtable: 4k pages, 48-bit VAs, pgdp=000000000757a000
>> >>>>> [ï¿½ï¿½ 21.686853] [00000000000000a0] pgd=0000000000000000, p4d=0000000000000000
>> >>>>> [ï¿½ï¿½ 21.687452] Internal error: Oops: 96000004EEMPT SMP
>> >>>>> [ï¿½ï¿½ 21.687941] Modules linked in:
>> >>>>> [ï¿½ï¿½ 21.688214] CPU: 4 PID: 1 Comm: shutdown Not tainted
>> >>>>> 5.12.0-rc7-00262-g568262bf5492 #33
>> >>>>> [ï¿½ï¿½ 21.688915] Hardware name: Pine64 RockPro64 v2.0 (DT)
>> >>>>> [ï¿½ï¿½ 21.689357] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
>> >>>>> [ï¿½ï¿½ 21.689884] pc : down_read_interruptible+0xec/0x200
>> >>>>> [ï¿½ï¿½ 21.690321] lr : simple_recursive_removal+0x48/0x280
>> >>>>> [ï¿½ï¿½ 21.690761] sp : ffff800011f4b940
>> >>>>> [ï¿½ï¿½ 21.691053] x29: ffff800011f4b940 x28: ffff000000809b40
>> >>>>> [ï¿½ï¿½ 21.691522] x27: ffff000000809b98 x26: ffff8000114f5170
>> >>>>> [ï¿½ï¿½ 21.691990] x25: 00000000000000a0 x24: ffff800011e84030
>> >>>>> [ï¿½ï¿½ 21.692459] x23: 0000000000000080 x22: 0000000000000000
>> >>>>> [ï¿½ï¿½ 21.692927] x21: ffff800011ecaa5c x20: ffff800011ecaa60
>> >>>>> [ï¿½ï¿½ 21.693395] x19: ffff000000809b40 x18: ffffffffffffffff
>> >>>>> [ï¿½ï¿½ 21.693863] x17: 0000000000000000 x16: 0000000000000000
>> >>>>> [ï¿½ï¿½ 21.694331] x15: ffff800091f4ba6d x14: 0000000000000004
>> >>>>> [ï¿½ï¿½ 21.694799] x13: 0000000000000000 x12: 0000000000000020
>> >>>>> [ï¿½ï¿½ 21.695267] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>> >>>>> [ï¿½ï¿½ 21.695735] x9 : 6f6c746364716e62 x8 : 7f7f7f7f7f7f7f7f
>> >>>>> [ï¿½ï¿½ 21.696203] x7 : fefefeff6364626d x6 : 0000000000001bd8
>> >>>>> [ï¿½ï¿½ 21.696671] x5 : 0000000000000000 x4 : 0000000000000000
>> >>>>> [ï¿½ï¿½ 21.697138] x3 : 00000000000000a0 x2 : 0000000000000001
>> >>>>> [ï¿½ï¿½ 21.697606] x1 : 0000000000000000 x0 : 00000000000000a0
>> >>>>> [ï¿½ï¿½ 21.698075] Call trace:
>> >>>>> [ï¿½ï¿½ 21.698291]ï¿½ down_read_interruptible+0xec/0x200
>> >>>>> [ï¿½ï¿½ 21.698690]ï¿½ debugfs_remove+0x60/0x84
>> >>>>> [ï¿½ï¿½ 21.699016]ï¿½ dwc3_debugfs_exit+0x1c/0x6c
>> >>>>> [ï¿½ï¿½ 21.699363]ï¿½ dwc3_remove+0x34/0x1a0
>> >>>>> [ï¿½ï¿½ 21.699672]ï¿½ platform_remove+0x28/0x60
>> >>>>> [ï¿½ï¿½ 21.700005]ï¿½ __device_release_driver+0x188/0x230
>> >>>>> [ï¿½ï¿½ 21.700414]ï¿½ device_release_driver+0x2c/0x44
>> >>>>> [ï¿½ï¿½ 21.700791]ï¿½ bus_remove_device+0x124/0x130
>> >>>>> [ï¿½ï¿½ 21.701154]ï¿½ device_del+0x168/0x420
>> >>>>> [ï¿½ï¿½ 21.701462]ï¿½ platform_device_del.part.0+0x1c/0x90
>> >>>>> [ï¿½ï¿½ 21.701877]ï¿½ platform_device_unregister+0x28/0x44
>> >>>>> [ï¿½ï¿½ 21.702291]ï¿½ of_platform_device_destroy+0xe8/0x100
>> >>>>> [ï¿½ï¿½ 21.702716]ï¿½ device_for_each_child_reverse+0x64/0xb4
>> >>>>> [ï¿½ï¿½ 21.703153]ï¿½ of_platform_depopulate+0x40/0x84
>> >>>>> [ï¿½ï¿½ 21.703538]ï¿½ __dwc3_of_simple_teardown+0x20/0xd4
>> >>>>> [ï¿½ï¿½ 21.703945]ï¿½ dwc3_of_simple_shutdown+0x14/0x20
>> >>>>> [ï¿½ï¿½ 21.704337]ï¿½ platform_shutdown+0x28/0x40
>> >>>>> [ï¿½ï¿½ 21.704683]ï¿½ device_shutdown+0x158/0x330
>> >>>>> [ï¿½ï¿½ 21.705029]ï¿½ kernel_power_off+0x38/0x7c
>> >>>>> [ï¿½ï¿½ 21.705372]ï¿½ __do_sys_reboot+0x16c/0x2a0
>> >>>>> [ï¿½ï¿½ 21.705719]ï¿½ __arm64_sys_reboot+0x28/0x34
>> >>>>> [ï¿½ï¿½ 21.706074]ï¿½ el0_svc_common.constprop.0+0x60/0x120
>> >>>>> [ï¿½ï¿½ 21.706499]ï¿½ do_el0_svc+0x28/0x94
>> >>>>> [ï¿½ï¿½ 21.706794]ï¿½ el0_svc+0x2c/0x54
>> >>>>> [ï¿½ï¿½ 21.707067]ï¿½ el0_sync_handler+0xa4/0x130
>> >>>>> [ï¿½ï¿½ 21.707414]ï¿½ el0_sync+0x170/0x180
>> >>>>> [ï¿½ï¿½ 21.707711] Code: c8047c62 35ffff84 17fffe5f f9800071 (c85ffc60)
>> >>>>> [ï¿½ï¿½ 21.708250] ---[ end trace 5ae08147542eb468 ]---
>> >>>>> [ï¿½ï¿½ 21.708667] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>> >>>>> [ï¿½ï¿½ 21.709456] Kernel Offset: disabled
>> >>>>> [ï¿½ï¿½ 21.709762] CPU features: 0x00240022,2100600c
>> >>>>> [ï¿½ï¿½ 21.710146] Memory Limit: 2048 MB
>> >>>>> [ï¿½ï¿½ 21.710443] ---[ end Kernel panic - not syncing: Attempted to kill init!
>> >>>>> exitcode=0x0000000b ]---
>> >>>>>
>> >>>>> I've been able to bisect the panic and the offending commit is 568262bf5492 ("usb:
>> >>>>> dwc3: core: Add shutdown callback for dwc3"). I can provide more diagnostic
>> >>>>> information if needed and I can help test the fix.
>> >>>> if you simply revert that commit in HEAD, does the problem really go
>> >>>> away?
>> >>> Kernel built from commit 324c92e5e0ee, which is the kernel tip today, the panic is
>> >>> there. Reverting the offending commit, 568262bf5492, makes the panic disappear.
>> >> Want to send a revert so I can take it now?
>> >
>> > I can send a revert, but Felipe was asking Sandeep (the commit author) for a fix,
>> > so I'll leave it up to Felipe to decide how to proceed.
>> 
>> I'm okay with a revert. Feel free to add my Acked-by: Felipe Balbi
>> <balbi@kernel.org> or it.
>> 
>> Sandeep, please send a new version that doesn't encounter the same
>> issue. Make sure to test by reloading the driver in a tight loop for
>> several iterations.
>
> This would probably be tricky to test on other "glue" drivers as the
> problem appears to be specific only to dwc3_of_simple.  It looks like
> both dwc3_of_simple and the dwc3 core now (due to 568262bf5492) each
> implement respective .shutdown callbacks. The latter is simply a wrapper
> around dwc3_remove(). And from the panic call stack above we see that
> dwc3_of_simple_shutdown() calls of_platform_depopulate() which will 
> again call dwc3_remove() resulting in the double remove.
>
> So would an alternative approach be to protect against dwc3_remove()
> getting called multiple times? IMO it'd be a bit messy to have to add

no, I  don't think so. That sounds like a workaround. We should be able
to guarantee that ->remove() doesn't get called twice using the driver
model properly.

> additional checks there to know if it had already been called. So maybe
> avoid it altogether--should dwc3_of_simple_shutdown() just skip calling
> of_platform_depopulate()?

I don't know what the idiomatic is nowadays, but at least early on, we
had to call depopulate.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]