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

[Louis H. Poulin II]

Hi Bjorn,

> 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().

Yes. The instrumentation shows bridge->link_state becoming NULL after
function-0 removal. Later lookups for surviving functions 0000:75:00.4
and 0000:75:00.6 still find upstream bridge 0000:00:08.1, but its
link_state is NULL.

This was on the ASUS TUF GAMING X870-PLUS WIFI, BIOS 1805, running
7.2.15-ogc1.1.fc45.aspm453.x86_64 (#453). It was built from Linux
7.2.15 plus the OpenGamingCollective downstream patch set and local
diagnostics. This is not a reproduction on unmodified upstream.

The diagnostic patch logs initialization, publication, visibility,
lookup and teardown, with dump_stack() in free_link_state(). It also
adds NULL checks returning -ENODEV to the ASPM and clock power-management
sysfs show/store callbacks so collection can continue. These checks
are diagnostic containment, not a proposed fix.

The trigger is a local systemd oneshot,
helix-gaming-disable-igpu.service. Its stated purpose is to keep the
iGPU display function out of Gamescope DRM enumeration. Its command is:

  /bin/sh -c 'if [ -e /sys/bus/pci/devices/0000:75:00.0/remove ]; then
echo 1 > /sys/bus/pci/devices/0000:75:00.0/remove; fi'

It targets function .0, not the bridge or siblings. In the same boot,
systemd retains ExecMainPID=2486, matching the shell PID in the stack.
Its recorded execution interval is 10.303980-10.348137 s monotonic;
the journal records exit-free at 10.308186 s, within that interval.
The dmesg timestamps in the separate excerpt below are left unchanged.

Stack excerpt from the dump_stack() in free_link_state():

  CPU: 1 UID: 0 PID: 2486 Comm: sh Not tainted
7.2.15-ogc1.1.fc45.aspm453.x86_64 #453 PREEMPT(lazy)
  dump_stack_lvl+0x5d/0x80
  pcie_aspm_exit_link_state.cold+0x5/0x14
  pci_remove_bus_device+0x10b/0x170
  pci_stop_and_remove_bus_device_locked+0x22/0x30
  remove_store+0x83/0xa0
  kernfs_fop_write_iter+0x178/0x200
  vfs_write+0x281/0x560
  ksys_write+0x7b/0x110

The matching vmlinux resolves the cold frame to free_link_state(),
inlined into pcie_aspm_exit_link_state().

Selected dmesg fields (abridged; attr= names the downstream device in
publish-link, free-link and clear-link records):

  [0.367481] seq=98 event=publish-link dev=0000:00:08.1
link_state=present attr=0000:75:00.0
  [0.367583] seq=159 event=visible dev=0000:75:00.4
bridge=0000:00:08.1 link_state=present attr=l0s_aspm arg=420
  [0.367609] seq=177 event=visible dev=0000:75:00.6
bridge=0000:00:08.1 link_state=present attr=l0s_aspm arg=420
  [10.307508] seq=191 event=exit-free dev=0000:75:00.0
bridge=0000:00:08.1 link_state=present
  [10.308089] seq=192 event=free-link dev=0000:00:08.1
link_state=present attr=0000:75:00.0
  [10.308728] seq=193 event=clear-link dev=0000:00:08.1
link_state=NULL attr=0000:75:00.0
  [10.896953] seq=194 event=get-bridge-link-null dev=0000:75:00.6
bridge=0000:00:08.1 link_state=NULL
  [10.896958] seq=195 event=show dev=0000:75:00.6 bridge=0000:00:08.1
link_state=NULL attr=l0s_aspm
  [10.896960] seq=196 event=show-result dev=0000:75:00.6
bridge=0000:00:08.1 link_state=NULL attr=l0s_aspm arg=-19
  [297.729199] seq=242 event=get-bridge-link-null dev=0000:75:00.4
bridge=0000:00:08.1 link_state=NULL
  [297.729204] seq=244 event=show-result dev=0000:75:00.4
bridge=0000:00:08.1 link_state=NULL attr=l0s_aspm arg=-19

arg=420 is mode 0644. l1_aspm was also exposed on .4 and .6 (seq 161
and 179); subsequent l1_aspm reads also reached the NULL lookup. The
-19 results above are produced by the added diagnostic guards. In
this source, the unguarded show path dereferences link without a check.

The trace shows publication for downstream .0, visibility with link
state present, then teardown and pointer clearing when .0 is removed.
Both siblings remain with reachable attributes. The first recorded
post-teardown NULL lookup on .6 is about 0.59 s after clear-link in
dmesg; .4 also reaches the NULL lookup at 297.729199 s.

This establishes teardown followed by NULL lookups. It does not
demonstrate a concurrent use-after-free or rule out other lifetime
issues. I have preserved the full logs, diagnostic patch, source
inputs, configuration and matching-vmlinux symbolization.

Thanks,
Louis


On Thu, 05 Nov 2026 17:46:28 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+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

[Bjorn Helgaas]

[+cc Daniel, Ding]

On Sun, Nov 08, 2026 at 12:36:30PM -0800, Louis H. Poulin II wrote:
> Hi Bjorn,
> 
> > 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().
> 
> Yes. The instrumentation shows bridge->link_state becoming NULL after
> function-0 removal. Later lookups for surviving functions 0000:75:00.4
> and 0000:75:00.6 still find upstream bridge 0000:00:08.1, but its
> link_state is NULL.
> 
> This was on the ASUS TUF GAMING X870-PLUS WIFI, BIOS 1805, running
> 7.2.15-ogc1.1.fc45.aspm453.x86_64 (#453). It was built from Linux
> 7.2.15 plus the OpenGamingCollective downstream patch set and local
> diagnostics. This is not a reproduction on unmodified upstream.
> 
> The diagnostic patch logs initialization, publication, visibility,
> lookup and teardown, with dump_stack() in free_link_state(). It also
> adds NULL checks returning -ENODEV to the ASPM and clock power-management
> sysfs show/store callbacks so collection can continue. These checks
> are diagnostic containment, not a proposed fix.
> 
> The trigger is a local systemd oneshot,
> helix-gaming-disable-igpu.service. Its stated purpose is to keep the
> iGPU display function out of Gamescope DRM enumeration. Its command is:
> 
>   /bin/sh -c 'if [ -e /sys/bus/pci/devices/0000:75:00.0/remove ]; then
> echo 1 > /sys/bus/pci/devices/0000:75:00.0/remove; fi'

Thanks for the debugging.  Before that remove, can you collect the
output of "sudo lspci -vv" so we have a picture of the whole topology?

It seems legit to remove one function of the multi-function device at
75:00.x (from a software perspective; obviously we can't physically
remove it separately from the other functions), but seems like we
prematurely discard the ASPM link info when doing so.  I haven't dug
into them, but maybe these commits have some bearing on this:

  456d8aa37d0f ("PCI/ASPM: Disable ASPM on MFD function removal to avoid use-after-free")
  cbf937dcadfd ("PCI/ASPM: Fix link state exit during switch upstream function removal")

> It targets function .0, not the bridge or siblings. In the same boot,
> systemd retains ExecMainPID=2486, matching the shell PID in the stack.
> Its recorded execution interval is 10.303980-10.348137 s monotonic;
> the journal records exit-free at 10.308186 s, within that interval.
> The dmesg timestamps in the separate excerpt below are left unchanged.
> 
> Stack excerpt from the dump_stack() in free_link_state():
> 
>   CPU: 1 UID: 0 PID: 2486 Comm: sh Not tainted
> 7.2.15-ogc1.1.fc45.aspm453.x86_64 #453 PREEMPT(lazy)
>   dump_stack_lvl+0x5d/0x80
>   pcie_aspm_exit_link_state.cold+0x5/0x14
>   pci_remove_bus_device+0x10b/0x170
>   pci_stop_and_remove_bus_device_locked+0x22/0x30
>   remove_store+0x83/0xa0
>   kernfs_fop_write_iter+0x178/0x200
>   vfs_write+0x281/0x560
>   ksys_write+0x7b/0x110
> 
> The matching vmlinux resolves the cold frame to free_link_state(),
> inlined into pcie_aspm_exit_link_state().
> 
> Selected dmesg fields (abridged; attr= names the downstream device in
> publish-link, free-link and clear-link records):
> 
>   [0.367481] seq=98 event=publish-link dev=0000:00:08.1
> link_state=present attr=0000:75:00.0
>   [0.367583] seq=159 event=visible dev=0000:75:00.4
> bridge=0000:00:08.1 link_state=present attr=l0s_aspm arg=420
>   [0.367609] seq=177 event=visible dev=0000:75:00.6
> bridge=0000:00:08.1 link_state=present attr=l0s_aspm arg=420
>   [10.307508] seq=191 event=exit-free dev=0000:75:00.0
> bridge=0000:00:08.1 link_state=present
>   [10.308089] seq=192 event=free-link dev=0000:00:08.1
> link_state=present attr=0000:75:00.0
>   [10.308728] seq=193 event=clear-link dev=0000:00:08.1
> link_state=NULL attr=0000:75:00.0
>   [10.896953] seq=194 event=get-bridge-link-null dev=0000:75:00.6
> bridge=0000:00:08.1 link_state=NULL
>   [10.896958] seq=195 event=show dev=0000:75:00.6 bridge=0000:00:08.1
> link_state=NULL attr=l0s_aspm
>   [10.896960] seq=196 event=show-result dev=0000:75:00.6
> bridge=0000:00:08.1 link_state=NULL attr=l0s_aspm arg=-19
>   [297.729199] seq=242 event=get-bridge-link-null dev=0000:75:00.4
> bridge=0000:00:08.1 link_state=NULL
>   [297.729204] seq=244 event=show-result dev=0000:75:00.4
> bridge=0000:00:08.1 link_state=NULL attr=l0s_aspm arg=-19
> 
> arg=420 is mode 0644. l1_aspm was also exposed on .4 and .6 (seq 161
> and 179); subsequent l1_aspm reads also reached the NULL lookup. The
> -19 results above are produced by the added diagnostic guards. In
> this source, the unguarded show path dereferences link without a check.
> 
> The trace shows publication for downstream .0, visibility with link
> state present, then teardown and pointer clearing when .0 is removed.
> Both siblings remain with reachable attributes. The first recorded
> post-teardown NULL lookup on .6 is about 0.59 s after clear-link in
> dmesg; .4 also reaches the NULL lookup at 297.729199 s.
> 
> This establishes teardown followed by NULL lookups. It does not
> demonstrate a concurrent use-after-free or rule out other lifetime
> issues. I have preserved the full logs, diagnostic patch, source
> inputs, configuration and matching-vmlinux symbolization.
> 
> Thanks,
> Louis
> 
> 
> On Thu, 05 Nov 2026 17:46:28 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+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