[PATCH] pci: Don't call resume callback for nearly bound devices

From: Uwe Kleine-König
Date: Mon Nov 08 2021 - 16:22:57 EST

pci_pm_runtime_resume() exits early when the device to resume isn't
bound yet:

if (!to_pci_driver(dev->driver))
return 0;

This however isn't true when the device currently probes and
local_pci_probe() calls pm_runtime_get_sync() because then the driver
core already setup dev->driver. As a result the driver's resume callback
is called before the driver's probe function is called and so more often
than not required driver data isn't setup yet.

So replace the check for the device being unbound by a check that only
becomes true after .probe() succeeded.

Thanks to Bjorn Helgaas for the analysis.

Reported-by: Robert Święcki <robert@xxxxxxxxxxx>
Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
---
On Mon, Nov 08, 2021 at 12:58:23PM -0600, Bjorn Helgaas wrote:
> [+cc Uwe, Rafael, linux-pm, linux-pci, linux-kernel, beginning of
> thread: https://lore.kernel.org/linux-i2c/CAP145pgdrdiMAT7=-iB1DMgA7t_bMqTcJL4N0=6u8kNY3EU0dw@xxxxxxxxxxxxxx/T/#t]
>
> On Mon, Nov 08, 2021 at 05:34:14PM +0100, Robert Święcki wrote:
> > > I'm daily-driving the linux from Linus' git (recompiling every day or
> > > two), and yesterday it stopped booting. Below is the dmesg from
> > > pstore.
> > > ...
> >
> > This introduced the bug: 0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> > https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
>
> Thank you very much for the debugging and this report! This report is
> for i2c, but the problem will affect many drivers.
>
> > > <1>[ 1.431369][ T447] BUG: kernel NULL pointer dereference,
> > > address: 0000000000000540
> > > <1>[ 1.431371][ T447] #PF: supervisor read access in kernel mode
> > > <1>[ 1.431375][ T447] #PF: error_code(0x0000) - not-present page
> > > <6>[ 1.431378][ T447] PGD 0 P4D 0
> > > <4>[ 1.431384][ T447] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > <4>[ 1.431388][ T447] CPU: 12 PID: 447 Comm: systemd-udevd
> > > Tainted: G E 5.15.0+ #91
> > > <4>[ 1.431391][ T447] Hardware name: ASUS System Product Name/ROG
> > > CROSSHAIR VIII FORMULA, BIOS 3801 07/30/2021
> > > <4>[ 1.431392][ T447] RIP: 0010:i2c_dw_pci_resume+0x8/0x40
> > > [i2c_designware_pci]
> > > <4>[ 1.431399][ T447] Code: 00 00 00 00 66 66 2e 0f 1f 84 00 00 00
> > > 00 00 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 84 00 00 00 00 00 53 48
> > > 8b 5f 78 48 89 df <ff> 93 40 05 00 00 c6 83 c0 05 00 00 00 5b c3 66 66
> > > 2e 0f 1f 84 00
> > > <4>[ 1.431401][ T447] RSP: 0018:ffffb3e740a13ba8 EFLAGS: 00010286
> > > <4>[ 1.431403][ T447] RAX: 0000000000000000 RBX: 0000000000000000
> > > RCX: 0000000000000000
>
> $ ./scripts/decodecode < oops
> 22: 53 push %rbx
> 23: 48 8b 5f 78 mov 0x78(%rdi),%rbx
> 27: 48 89 df mov %rbx,%rdi
> 2a:* ff 93 40 05 00 00 callq *0x540(%rbx) <-- trapping instruction
> 30: c6 83 c0 05 00 00 00 movb $0x0,0x5c0(%rbx)
> 37: 5b pop %rbx
> 38: c3 retq
>
> static int i2c_dw_pci_resume(struct device *dev)
> {
> struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> int ret;
>
> ret = i_dev->init(i_dev);
> i_dev->suspended = false;
>
> return ret;
>
> So I think we're trying to call i_dev->init(), which is a NULL
> pointer.
>
> > > <4>[ 1.431422][ T447] pci_pm_runtime_resume+0xaa/0x100
> > > <4>[ 1.431434][ T447] __rpm_callback+0x3c/0x100
> > > <4>[ 1.431442][ T447] rpm_callback+0x54/0x80
> > > <4>[ 1.431445][ T447] rpm_resume+0x410/0x700
> > > <4>[ 1.431455][ T447] __pm_runtime_resume+0x45/0x80
> > > <4>[ 1.431457][ T447] pci_device_probe+0xa2/0x140
> > > <4>[ 1.431459][ T447] really_probe+0x1e4/0x400
> > > <4>[ 1.431464][ T447] __driver_probe_device+0xf9/0x180
> > > <4>[ 1.431466][ T447] driver_probe_device+0x19/0xc0
>
> I think the problem here is that:
>
> - really_probe() sets dev->driver
>
> - local_pci_probe() calls pm_runtime_get_sync(), which leads to:
>
> - pci_pm_runtime_resume(), which previously skipped the driver's
> .runtime_resume() method when "pci_dev->driver" as NULL
>
> - after 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
> pci_dev->driver") [1], it checks "dev->driver" instead of
> "pci_dev->driver"
>
> - dev->driver is non-NULL (set by really_probe() above), but at this
> point pci_dev->driver used to be NULL because local_pci_probe()
> didn't set it until after after calling pm_runtime_get_sync() (see
> b5f9c644eb1b ("PCI: Remove struct pci_dev->driver") [2])
>
> - because dev->driver is non-NULL, we call i2c_dw_pci_resume()
> before i2c_dw_pci_probe(), so the driver init hasn't been done
>
> Here's the call tree:
>
> really_probe
> dev->driver = drv; # <--
> call_driver_probe
> dev->bus->probe
> pci_device_probe
> __pci_device_probe
> pci_call_probe
> local_pci_probe
> pm_runtime_get_sync
> ...
> pci_pm_runtime_resume
> - if (!pci_dev->driver) # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> + if (!to_pci_driver(dev->driver))
> return 0
> pm->runtime_resume
> i2c_dw_pci_resume
> i_dev->init() # <-- NULL ptr deref
> - pci_dev->driver = pci_drv # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
> pci_drv->probe
> i2c_dw_pci_probe

I think this analysis is right.

I didn't test this patch, @Robert, maybe you can do this?

Best regards
Uwe

drivers/pci/pci-driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1d98c974381c..202533654012 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1299,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
*/
pci_restore_standard_config(pci_dev);

- if (!to_pci_driver(dev->driver))
+ if (!device_is_bound(dev))
return 0;

pci_fixup_device(pci_fixup_resume_early, pci_dev);
--
2.30.2


--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature