Regression: qca8k_sw_probe crashes (Was: Re: [net-next PATCH v5 01/14] net: dsa: qca8k: cache match data to speed up access)

Pali Rohár
2022-09-14T20:06:54Z

On Wednesday 27 July 2022 13:35:10 Christian Marangi wrote:
> Using of_device_get_match_data is expensive. Cache match data to speed
> up access and rework user of match data to use the new cached value.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> drivers/net/dsa/qca/qca8k.c | 35 +++++++++++------------------------
> drivers/net/dsa/qca/qca8k.h | 1 +
> 2 files changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
> index 1cbb05b0323f..64524a721221 100644

Hello! This commit is causing kernel crash on powerpc P2020 based board
with QCA8337N-AL3C switch.

[ 1.901926] Kernel attempted to read user page (38) - exploit attempt? (uid: 0)
[ 1.909264] BUG: Kernel NULL pointer dereference on read at 0x00000038
[ 1.915793] Faulting instruction address: 0xc079adec
[ 1.920756] Oops: Kernel access of bad area, sig: 11 [#1]
[ 1.926154] BE PAGE_SIZE=4K SMP NR_CPUS=2 P2020RDB-PC
[ 1.931207] Modules linked in:
[ 1.934260] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2-0caacb197b677410bdac81bc34f05235+ #123
[ 1.943576] NIP: c079adec LR: c06b8920 CTR: 00000000
[ 1.948626] REGS: c146f970 TRAP: 0300 Not tainted (6.0.0-rc2-0caacb197b677410bdac81bc34f05235+)
[ 1.957591] MSR: 00029000 <CE,EE,ME> CR: 84228842 XER: 20000000
[ 1.963786] DEAR: 00000038 ESR: 00000000
[ 1.963786] GPR00: c06b8920 c146fa60 c14a8000 00000000 00000000 00000000 c1c957fc 00000004
[ 1.963786] GPR08: c174f6e8 00000000 c1c95700 c18c2e40 84228848 00000000 c0004548 00000000
[ 1.963786] GPR16: 00000000 c174f600 00000000 fffff000 c0c06504 c0c05c3c efff8b9c efff8b9c
[ 1.963786] GPR24: 00000001 efff8bb0 efff8bac 00000000 00000000 c174f600 00000000 c1c95718
[ 2.001278] NIP [c079adec] of_device_get_match_data+0x10/0x38
[ 2.007040] LR [c06b8920] qca8k_sw_probe+0x40/0x224
[ 2.011927] Call Trace:
[ 2.014368] [c146fa60] [c174f600] 0xc174f600 (unreliable)
[ 2.019774] [c146fa70] [c06b8920] qca8k_sw_probe+0x40/0x224
[ 2.025354] [c146faa0] [c06b2ef0] mdio_probe+0x40/0x8c
[ 2.030497] [c146fac0] [c060ddb8] really_probe+0x250/0x340
[ 2.035990] [c146faf0] [c060dfb0] driver_probe_device+0x44/0xfc
[ 2.041916] [c146fb20] [c060e440] __device_attach_driver+0xa4/0x114
[ 2.048190] [c146fb40] [c060b83c] bus_for_each_drv+0x8c/0xe8
[ 2.053855] [c146fb70] [c060db04] __device_attach+0x110/0x164
[ 2.059607] [c146fba0] [c060c8ec] bus_probe_device+0xb0/0xd4
[ 2.065271] [c146fbc0] [c0609654] device_add+0x438/0x8c8
[ 2.070595] [c146fc20] [c06b3018] mdio_device_register+0x48/0x70
[ 2.076608] [c146fc40] [c06b4c98] of_mdiobus_register+0x310/0x398
[ 2.082707] [c146fca0] [c06ba7d4] fsl_pq_mdio_probe+0x1f0/0x374
[ 2.088636] [c146fd10] [c0610708] platform_probe+0x48/0xac
[ 2.094129] [c146fd30] [c060dc40] really_probe+0xd8/0x340
[ 2.099533] [c146fd60] [c060dfb0] driver_probe_device+0x44/0xfc
[ 2.105459] [c146fd90] [c060e534] __driver_attach+0x84/0x154
[ 2.111124] [c146fdb0] [c060b73c] bus_for_each_dev+0x88/0xe0
[ 2.116789] [c146fde0] [c060cc80] bus_add_driver+0x1f0/0x22c
[ 2.122453] [c146fe10] [c060eca8] driver_register+0x88/0x16c
[ 2.128118] [c146fe30] [c0004150] do_one_initcall+0x80/0x284
[ 2.133785] [c146fea0] [c10012d0] kernel_init_freeable+0x1f4/0x2a0
[ 2.139973] [c146fee0] [c000456c] kernel_init+0x24/0x154
[ 2.145289] [c146ff00] [c001426c] ret_from_kernel_thread+0x5c/0x64
[ 2.151479] Instruction dump:
[ 2.154444] 71290020 40820010 7d445378 38210010 4bffe068 38600000 38210010 4e800020
[ 2.162211] 9421fff0 7c0802a6 7c641b78 90010014 <81230038> 80690018 4bffffad 2c030000
[ 2.170156] ---[ end trace 0000000000000000 ]---
[ 2.174770]
[ 3.176268] Kernel panic - not syncing: Fatal exception
[ 3.181497] Rebooting in 1 seconds..

> --- a/drivers/net/dsa/qca/qca8k.c
> +++ b/drivers/net/dsa/qca/qca8k.c

There is line:
priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
and after that follows chunk:

> @@ -3134,6 +3120,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> if (!priv)
> return -ENOMEM;
>
> + priv->info = of_device_get_match_data(priv->dev);

So function of_device_get_match_data() takes as its argument NULL
pointer as 'priv' structure is at this stage zeroed, and which cause
above kernel crash. priv->dev is filled lines below:

> priv->bus = mdiodev->bus;
> priv->dev = &mdiodev->dev;
>

I would propose following patch:

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 1d3e7782a71f..614950a5878a 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1887,13 +1887,13 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
*/
priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;

+ priv->dev = &mdiodev->dev;
priv->info = of_device_get_match_data(priv->dev);
priv->bus = mdiodev->bus;
- priv->dev = &mdiodev->dev;

priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
GPIOD_ASIS);
if (IS_ERR(priv->reset_gpio))
return PTR_ERR(priv->reset_gpio);

which fixes above crash.