Re: [PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

Re: [PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

[Bjorn Helgaas]

[+cc other members of conversation at
https://lore.kernel.org/all/20260727072915.3954329-1-zhaojinming@uniontech.com]

On Wed, Nov 04, 2026 at 03:22:51PM -0800, Poulin, Louis wrote:
> Hi,
> 
> We observed a cold-boot crash in l0s_aspm_show() that appears to
> contradict the assumption in this thread that the visibility check
> always keeps readers out when the ASPM link is unavailable.
> 
> This was on an ASUS TUF GAMING X870-PLUS WIFI, BIOS 1805, running
> Bazzite's 7.2.15-ogc1.1.fc45.x86_64 kernel with InputPlumber 0.83.1.
> The fault occurred about 10 seconds into a cold boot. An earlier boot
> on the same system had the same fault address and symbol offset on
> 7.2.9-ogc3.1.fc44.x86_64 with InputPlumber 0.83.0. Both observations
> were on downstream kernels. The captured 7.2.15 kernel was tainted
> O/E (out-of-tree and unsigned module).
> 
> Relevant excerpt from 7.2.15:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000030
>   Comm: inputplumber
>   RIP: 0010:l0s_aspm_show+0x47/0x80
>   RAX: 0000000000000000
>   CR2: 0000000000000030
>   Call Trace:
>     dev_attr_show+0x1f/0x50
>     sysfs_kf_seq_show+0xa8/0x110
>     seq_read_iter+0x132/0x4c0
>     vfs_read+0x34d/0x490
> 
> The faulting instruction accesses offset 0x30 from the NULL register.
> In the corresponding source, aspm_attr_show_common() dereferences the
> result of pcie_aspm_get_link(). The read reached the callback even
> though that lookup had no link to return. The kernel killed the
> InputPlumber main thread and the service stopped answering
> ObjectManager.GetManagedObjects. This blocked the boot-time controller
> setup.
> 
> We are building a diagnostic kernel that returns -ENODEV in that
> specific read callback when the link is absent and logs the PCI device
> and upstream bridge. The device and the reason its link state was
> missing are still unknown. We have not reproduced the fault on an
> unmodified upstream kernel.
> 
> Would this trace justify a guard in the read callback, or is there a
> preferred way to instrument link-state teardown and sysfs visibility
> on this hardware? We can follow up with the device and bridge once
> captured.

I don't think we should add a guard in addition to
aspm_ctrl_attrs_are_visible().

pcie_aspm_get_link() must have returned non-NULL in
aspm_ctrl_attrs_are_visible() for the sysfs file to be visible.

If it later returned NULL in aspm_attr_show_common(), we must have a
bug, like a lifetime issue or some kind of corruption.  Adding a guard
would just hide the bug.

Your next message suggests that the device and upstream bridge are
valid, so possibly bridge->link_state was NULL.  I would instrument
pcie_aspm_get_link() to verify that and add a dump_stack() in
free_link_state().

> Kernel source for the captured build
> https://github.com/OpenGamingCollective/linux/blob/f6857d4bfd614b90302903490796ef376be0a67d/drivers/pci/pcie/aspm.c
> 
> Thanks,
> Louis

Re: [PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

[Lukas Wunner]

On Mon, Jul 27, 2026 at 03:29:15PM +0800, ZhaoJinming wrote:
> The aspm_attr_show_common(), aspm_attr_store_common(), clkpm_show()
> and clkpm_store() sysfs callbacks call pcie_aspm_get_link() without
> checking the return value. A NULL return would indicate the device is
> not PCIe, has no upstream bridge, or the link state has not been
> initialized.
> 
> While the aspm_ctrl_attrs_are_visible() is_visible callback already
> hides these attributes when link is NULL, add the NULL check for
> defense in depth and consistency with other callers of
> pcie_aspm_get_link() in the same file (__pci_disable_link_state,
> __pci_enable_link_state, pcie_aspm_enabled).

Well, if we don't need the checks because the ->is_visible() callback
already hides the attributes, then we shouldn't add them.

Otherwise it's quite possible that someone else comes along and deletes
the checks again because they're superfluous.  

Thanks,

Lukas

[PATCH] PCI/ASPM: Add missing NULL check for link in sysfs show/store callbacks

[ZhaoJinming]

The aspm_attr_show_common(), aspm_attr_store_common(), clkpm_show()
and clkpm_store() sysfs callbacks call pcie_aspm_get_link() without
checking the return value. A NULL return would indicate the device is
not PCIe, has no upstream bridge, or the link state has not been
initialized.

While the aspm_ctrl_attrs_are_visible() is_visible callback already
hides these attributes when link is NULL, add the NULL check for
defense in depth and consistency with other callers of
pcie_aspm_get_link() in the same file (__pci_disable_link_state,
__pci_enable_link_state, pcie_aspm_enabled).

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/pci/pcie/aspm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 172783e7f519..108d151a786e 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1654,6 +1654,9 @@ static ssize_t aspm_attr_show_common(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
+	if (!link)
+		return 0;
+
 	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
 }
 
@@ -1665,6 +1668,9 @@ static ssize_t aspm_attr_store_common(struct device *dev,
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 	bool state_enable;
 
+	if (!link)
+		return -ENODEV;
+
 	if (kstrtobool(buf, &state_enable) < 0)
 		return -EINVAL;
 
@@ -1713,6 +1719,9 @@ static ssize_t clkpm_show(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
+	if (!link)
+		return 0;
+
 	return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
 }
 
@@ -1724,6 +1733,9 @@ static ssize_t clkpm_store(struct device *dev,
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 	bool state_enable;
 
+	if (!link)
+		return -ENODEV;
+
 	if (kstrtobool(buf, &state_enable) < 0)
 		return -EINVAL;
 
-- 
2.20.1