Re: [PATCH] PCI/ASPM: Call pcie_aspm_sanity_check() as late as possible

From: Stephen Kitt <steve@sk2.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org,
	Jan Rueth <rueth@comsys.rwth-aachen.de>
Subject: Re: [PATCH] PCI/ASPM: Call pcie_aspm_sanity_check() as late as possible
Date: Thu, 8 Dec 2022 09:00:17 +0100	[thread overview]
Message-ID: <20221208090017.132db7bd@heffalump.sk2.org> (raw)
In-Reply-To: <20221207215608.GA1471870@bhelgaas>

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

Hi Bjorn,

On Wed, 7 Dec 2022 15:56:08 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Oct 06, 2022 at 01:59:50PM +0200, Stephen Kitt wrote:
> > In pcie_aspm_init_link_state(), a number of checks are made to
> > determine whether the function should proceed, before the result of
> > the call to pcie_aspm_sanity_check() is actually used. The latter
> > function doesn't change any state, it only reports a result, so
> > calling it later doesn't make any difference to the state of the
> > devices or the information we have about them. But having the call
> > early reportedly can cause null-pointer dereferences; see
> > https://unix.stackexchange.com/q/322337 for one example with
> > pcie_aspm=off (this was reported in 2016, but the relevant code hasn't
> > changed since then).  
> 
> Thanks, Stephen!
> 
> That stackexchange report doesn't have much information, but it looks
> similar to this old report from Jan Rueth, which I'm sorry to say I
> never got resolved:
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=187731
>   https://lore.kernel.org/all/4cec62c2-218a-672b-8c12-d44e8df56aae@comsys.rwth-aachen.de/#t
> 
> And Jan's patch is almost identical to yours :)
> 
> I hope to get this resolved, but I don't have time to work on it
> before the upcoming merge window, which will probably open Sunday.
> And then it's holiday time, so it may be January before I get back to
> it.  I'm just dropping the links here as breadcrumbs for picking this
> back up.

Thanks for the update! I was somewhat bemused by the dereference here, I’m
reassured to see I’m not the only one. Unfortunately I don’t have hardware
which exhibits this problem, I submitted the patch because it seemed
reasonably sensible even though as you say there is probably something else
going on here. Of course if this approach is useful, Jan’s patch should go in
rather than mine.

Anyway, it’s been six years, so a few more weeks won’t make any difference
;-).

Enjoy the holiday season!

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]