Re: [PATCH v3] pci_crash: capture PCI config space at panic time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `pci_dev_put()` from an RCU softirq callback can sleep in atomic context.
- [High] Accessing `bus->ops->read` on a removed device causes a Use-After-Free if the host controller driver was unloaded.
- [Medium] Skipped captures export un-flushed zeroed buffers, allowing the crash parser to read stale crash data from RAM.
--

--- Patch [1]: [PATCH v3] pci_crash: capture PCI config space at panic time ---
commit 93c2fd5e7de948b1f47b018b3d8a6d93723570c6
Author: Johannes Hange <hangej@amazon.com>

pci_crash: capture PCI config space at panic time

This commit adds CONFIG_PCI_CRASH to capture PCI configuration space during
a panic before kexec. It uses a pre-allocated buffer exported via
VMCOREINFO and relies on RCU snapshots and lockless accesses to safely
record device state.

> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -131,7 +131,7 @@ int pci_bus_read_config_dword_trylock(struct pci_bus *bus, unsigned int devfn,
>  	if (!pci_trylock_config(flags)) {
>  		PCI_SET_ERROR_RESPONSE(value);
>  		return PCIBIOS_SET_FAILED;
>  	}
>  	res = bus->ops->read(bus, devfn, pos, 4, &data);

[Severity: High]
If a host controller driver is unloaded right before a panic, could this
dereference freed module memory?

Because the bus notifier uses a 200ms debounce delay, the crash snapshot
might hold a stale pci_dev reference. The reference keeps the pci_dev
struct alive, but if the controller module was unloaded, bus->ops->read
points to freed memory. Clean removals do not set error_state, so the
pci_crash_endpoint_reachable() check might not filter the device out.

[ ... ]

> diff --git a/kernel/pci_crash.c b/kernel/pci_crash.c
> --- a/kernel/pci_crash.c
> +++ b/kernel/pci_crash.c
> @@ -142,6 +142,6 @@ static void pci_crash_snapshot_free_rcu(struct rcu_head *head)
>  	if (READ_ONCE(pci_crash_captured_snap) == s)
>  		return;
>  
>  	for (i = 0; i < s->num_devs; i++)
>  		if (s->devs && s->devs[i])
>  			pci_dev_put(s->devs[i]);

[Severity: High]
Can this call to pci_dev_put() sleep in atomic context?

This runs in an RCU softirq callback. If a device was hot-removed,
this snapshot might hold the final reference to the device. Dropping
the last reference triggers device_release() -> pci_release_dev() ->
devres_release_all(), which may take sleeping locks or allocate memory
with GFP_KERNEL.

[ ... ]

> @@ -864,14 +864,14 @@ void pci_crash_save(void)
>  	WRITE_ONCE(pci_crash_captured_snap, s);
>  
>  	pci_crash_buffer = s->buffer;
>  	pci_crash_buffer_size = s->buffer_size;
>  	pci_crash_pagemap_phys = s->pagemap_phys;
>  
>  	cflags = READ_ONCE(capture_flags);
>  	if (!(cflags & PCI_CRASH_CAPTURE_ALWAYS)) {
>  		if (!(cflags & PCI_CRASH_CAPTURE_AER)) {
>  			goto out;
>  		}
>  		if (!pci_crash_aer_error_present(s)) {
>  			pr_info("no PCI errors detected, skipping capture\n");
>  			goto out;
>  		}
>  	}

[Severity: Medium]
If the capture is skipped and jumps to the out label, could the crash
parser read stale data from RAM?

The export variables are set before the AER check. If no AER errors are
found, the code jumps out without flushing the dcache. Since the buffer
and pagemap were written in process context during rebuild, they might
only reside in the CPU cache.

The crash kernel will read physical RAM, which might contain an older,
valid PCIC magic signature from a previous boot or panic, leading to
stale records being parsed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714175305.2783480-1-hangej@amazon.com?part=1