linux-kernel - Re: [PATCH 1/1] PCI/ASPM: Fix L1SS saving

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250205133707.GA910099@bhelgaas>
Date: Wed, 5 Feb 2025 07:37:07 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Jian-Hong Pan <jhp@...lessos.org>,
	linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	Niklāvs Koļesņikovs <pinkflames.linux@...il.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH 1/1] PCI/ASPM: Fix L1SS saving

On Wed, Feb 05, 2025 at 10:38:24AM +0200, Ilpo Järvinen wrote:
> On Tue, 4 Feb 2025, Bjorn Helgaas wrote:
> 
> > [+cc Rafael]
> > 
> > On Fri, Jan 31, 2025 at 05:29:13PM +0200, Ilpo Järvinen wrote:
> > > The commit 1db806ec06b7 ("PCI/ASPM: Save parent L1SS config in
> > > pci_save_aspm_l1ss_state()") aimed to perform L1SS config save for both
> > > the Upstream Port and its upstream bridge when handling an Upstream
> > > Port, which matches what the L1SS restore side does. However,
> > > parent->state_saved can be set true at an earlier time when the
> > > upstream bridge saved other parts of its state. 
> > 
> > So I guess the scenario is that we got here because some driver called
> > pci_save_state(pdev):
> > 
> >   pci_save_state
> >     dev->state_saved = true                <--
> >     pci_save_pcie_state
> >       pci_save_aspm_l1ss_state
> >         if (pcie_downstream_port(pdev))
> >           return
> >         # save pdev L1SS state here
> >         if (parent->state_saved)           <--
> >           return
> >         # save parent L1SS state here
> > 
> > and the problem is that we previously called pci_save_state(parent),
> > which set "parent->state_saved = true" but did not save its L1SS state
> > because pci_save_aspm_l1ss_state() is a no-op for Downstream Ports,
> > right?
> 
> Yes! An unfortunate interaction between those two checks.
> 
> > But I would think this would be a very common situation because
> > pcie_portdrv_probe() calls pci_save_state() for Downstream Ports when
> > pciehp, AER, PME, etc, are enabled, and this would happen before the
> > pci_save_state() calls from Endpoint drivers.
> > 
> > So I'm a little surprised that this didn't blow up for everybody
> > immediately.  Is there something that makes this unusual?
> 
> I agree it should be very common and was quite surprised that -next
> did not catch it. What I recall though is you modified the patch while 
> applying it by adding those Downstream Port checks so the fix 
> patch's Tested-by was for different code from what got applied (and it 
> would have been caught would the original author have tested also the 
> modified commit).

Sigh, that makes sense, it's probably my fault, sorry.  I apologize
for messing this up and causing all this extra work.

I applied this to pci/for-linus yesterday, so it should make it for
v6.14-rc2.

> Unfortunately, I cannot think of anything that would be so unusual to 
> warrant not detecting it earlier. Maybe it was just the holiday period 
> causing less testing and lower level of awareness in general? The machine 
> doesn't always hang because of the problem as was the case with Niklāvs,
> so it might have occurred but went unnoticed if it occurred for a device 
> that is not critical.
> 
> > > Then later when
> > > attempting to save the L1SS config while handling the Upstream Port,
> > > parent->state_saved is true in pci_save_aspm_l1ss_state() resulting in
> > > early return and skipping saving bridge's L1SS config because it is
> > > assumed to be already saved. Later on restore, junk is written into
> > > L1SS config which causes issues with some devices.
> > > 
> > > Remove parent->state_saved check and unconditionally save L1SS config
> > > also for the upstream bridge from an Upstream Port which ought to be
> > > harmless from correctness point of view. With the Upstream Port check
> > > now present, saving the L1SS config more than once for the bridge is no
> > > longer a problem (unlike when the parent->state_saved check got
> > > introduced into the fix during its development).
> > > 
> > > Fixes: 1db806ec06b7 ("PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()")
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219731
> > > Reported-by: Niklāvs Koļesņikovs <pinkflames.linux@...il.com>
> > > Tested-by: Niklāvs Koļesņikovs <pinkflames.linux@...il.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > > ---
> > >  drivers/pci/pcie/aspm.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > index e0bc90597dca..da3e7edcf49d 100644
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -108,9 +108,6 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> > >  	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
> > >  	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
> > >  
> > > -	if (parent->state_saved)
> > > -		return;
> > > -
> > >  	/*
> > >  	 * Save parent's L1 substate configuration so we have it for
> > >  	 * pci_restore_aspm_l1ss_state(pdev) to restore.
> > > 
> > > base-commit: 72deda0abee6e705ae71a93f69f55e33be5bca5c
> > > -- 
> > > 2.39.5
> > > 
> > 
> 
> -- 
>  i.