Re: [PATCH] PCI/pwrctl: Do not assume device node presence

Re: [PATCH] PCI/pwrctl: Do not assume device node presence

[Bjorn Helgaas]

[+cc OF folks]

On Thu, Nov 21, 2024 at 05:40:19PM +0800, Chen-Yu Tsai wrote:
> A PCI device normally does not have a device node, since the bus is
> fully enumerable. Assuming that a device node is presence is likely
> bad.

> The newly added pwrctl code assumes such and crashes with a NULL
> pointer dereference.

> Besides that, of_find_device_by_node(NULL)
> is likely going to return some random device.

I thought this sounded implausible, but after looking at the code, I
think you're right, because bus_find_device() will use
device_match_of_node(), which decides the device matches if
"dev->of_node == np" (where "np" is NULL in this case).

I'm sure many devices will have "dev->of_node == NULL", so it does
seem like of_find_device_by_node(NULL) will return the first one it
finds.

This seems ... pretty janky and unexpected to me, but it's been this
way for years, so maybe it's safe?  Cc'ing the OF folks just in case.

> Reported-by: Klara Modin <klarasmodin@gmail.com>
> Closes: https://lore.kernel.org/linux-pci/a7b8f84d-efa6-490c-8594-84c1de9a7031@gmail.com/
> Fixes: cc70852b0962 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers")
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Krzysztof Wilczyński <kwilczynski@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: stable+noautosel@kernel.org         # Depends on power supply check
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/pci/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 98910bc0fcc4..eca72e0c3b6c 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -405,7 +405,7 @@ void pci_bus_add_device(struct pci_dev *dev)
>  	 * before PCI client drivers.
>  	 */
>  	pdev = of_find_device_by_node(dn);
> -	if (pdev && of_pci_supply_present(dn)) {
> +	if (dn && pdev && of_pci_supply_present(dn)) {

Thanks for this fix.  Krzysztof restored the NULL pointer check in
of_pci_supply_present(), so of_pci_supply_present(NULL) will return
false, which should also solve this problem.

If of_find_device_by_node(NULL) returned NULL, we wouldn't need this,
but for now I guess we do.

>  		if (!device_link_add(&dev->dev, &pdev->dev,
>  				     DL_FLAG_AUTOREMOVE_CONSUMER))
>  			pci_err(dev, "failed to add device link to power control device %s\n",
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH] PCI/pwrctl: Do not assume device node presence

[Rob Herring]

On Thu, Nov 21, 2024 at 10:46 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc OF folks]
>
> On Thu, Nov 21, 2024 at 05:40:19PM +0800, Chen-Yu Tsai wrote:
> > A PCI device normally does not have a device node, since the bus is
> > fully enumerable. Assuming that a device node is presence is likely
> > bad.
>
> > The newly added pwrctl code assumes such and crashes with a NULL
> > pointer dereference.
>
> > Besides that, of_find_device_by_node(NULL)
> > is likely going to return some random device.
>
> I thought this sounded implausible, but after looking at the code, I
> think you're right, because bus_find_device() will use
> device_match_of_node(), which decides the device matches if
> "dev->of_node == np" (where "np" is NULL in this case).
>
> I'm sure many devices will have "dev->of_node == NULL", so it does
> seem like of_find_device_by_node(NULL) will return the first one it
> finds.
>
> This seems ... pretty janky and unexpected to me, but it's been this
> way for years, so maybe it's safe?  Cc'ing the OF folks just in case.

This is a surprise to me, too. I think ACPI matching is broken in this
way too. I'm sending out a fix.

Rob