Received-SPF: pass (google.com: domain of linux-kernel+bounces-190597-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 27 May 2024 14:33:22 +0200
From: Lukas Wunner <lukas@wunner.de>
To: Nam Cao <namcao@linutronix.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>, Yinghai Lu <yinghai@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH v2 2/2] PCI: pciehp: Abort hot-plug if
 pci_hp_add_bridge() fails
Message-ID: <ZlR9kg-SEshXvBEP@wunner.de>
References: <cover.1714838173.git.namcao@linutronix.de>
 <f3db713f4a737756782be6e94fcea3eda352e39f.1714838173.git.namcao@linutronix.de>
 <Zjcc6Suf5HmmZVM9@wunner.de>
 <20240505071451.df3l6mdK@linutronix.de>
 <20240506083701.NZNifFGn@linutronix.de>
 <ZjkxTGaAc48jPzqC@wunner.de>
 <20240507142738.wyj19VVh@linutronix.de>
 <ZlRPS9TCYjccpNLr@wunner.de>
 <20240527092322.N8nbxYAL@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240527092322.N8nbxYAL@linutronix.de>

On Mon, May 27, 2024 at 11:23:22AM +0200, Nam Cao wrote:
> On Mon, May 27, 2024 at 11:15:55AM +0200, Lukas Wunner wrote:
> > We already check for a NULL subordinate pointer in various places.
> > See e.g. commit 62e4492c3063 ("PCI: Prevent NULL dereference during
> > pciehp probe").
> 
> Ah, so bridge without subordinate bus is allowed in the kernel.
> 
> > If we're missing such checks, I'd suggest to add those.
> > 
> > If you believe having a NULL subordinate pointer is wrong and the
> > bridge should be de-enumerated altogether, I think you would have
> > to remove these NULL pointer checks as they'd otherwise become
> > pointless with your change.
> > 
> > Just adding missing NULL pointer checks seems to be the most
> > straightforward solution to me.
> 
> If the kernel do permits bridges without subordinate bus number, I am
> happy to go this direction. I expect going this way will require many more
> patches, I will dig into it and come back later.

It seems a lot of functions use a "if (dev->subordinate)" check
to distinguish between bridges and endpoints.

A bridge with a NULL subordinate pointer is then basically treated
like an endpoint.

There are places which use other ways to recognize bridges, e.g.
by looking at dev->hdr_type.  Only these code paths will have to
check for a NULL subordinate pointer.  In the case of pciehp,
portdrv binds to anything with class PCI_CLASS_BRIDGE_PCI_NORMAL,
without consideration for the subordinate pointer, hence a check
was needed in pciehp.

pciehp is used a lot by Thunderbolt/USB4, SD 7.0 and for NVMe drives.
I think shpchp isn't used as much, so its code paths aren't exercised
as heavily and bugs aren't found and fixed as quickly.  So chances are
that more checks are missing in shpchp than in pciehp.

Thanks,

Lukas