It recently came up that of_find_device_by_node() will match a device
with a NULL of_node pointer. This is not desired behavior. The returned
struct device is also not deterministic.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
It would be a bit more efficient to check this up front before we
iterate thru devices, but there's a number of users of these functions
and this isn't really a hot path.

I think at least device_match_acpi_dev() and device_match_acpi_handle()
should also be fixed, but am not sure about the ACPI side.
---
 drivers/base/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 94865c9d8adc..87d50c5f710f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5246,13 +5246,13 @@ EXPORT_SYMBOL_GPL(device_match_name);
 
 int device_match_of_node(struct device *dev, const void *np)
 {
-	return dev->of_node == np;
+	return np && (dev->of_node == np);
 }
 EXPORT_SYMBOL_GPL(device_match_of_node);
 
 int device_match_fwnode(struct device *dev, const void *fwnode)
 {
-	return dev_fwnode(dev) == fwnode;
+	return fwnode && (dev_fwnode(dev) == fwnode);
 }
 EXPORT_SYMBOL_GPL(device_match_fwnode);
 
-- 
2.45.2

On Tue, Dec 03, 2024 at 06:02:59PM -0600, Rob Herring (Arm) wrote:
> It recently came up that of_find_device_by_node() will match a device
> with a NULL of_node pointer. This is not desired behavior. The returned
> struct device is also not deterministic.

It's not deterministic because a NULL pointer will cause that to happen,
or for some other reason?

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> It would be a bit more efficient to check this up front before we
> iterate thru devices, but there's a number of users of these functions
> and this isn't really a hot path.

Yeah, this should be fine.  Does this fix a problem now and we need it
merged for 6.13-final and backported, or can it just wait for 6.14-rc1?

thanks,

greg k-h

Hi Greg,

On Wed, Dec 04, 2024 at 10:37:06AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 03, 2024 at 06:02:59PM -0600, Rob Herring (Arm) wrote:
> > It recently came up that of_find_device_by_node() will match a device
> > with a NULL of_node pointer. This is not desired behavior. The returned
> > struct device is also not deterministic.
> 
> It's not deterministic because a NULL pointer will cause that to happen,
> or for some other reason?

It'll pick the first platform device with no of_node. That likely yields
something very wrong, but doesn't produce a visible problem until a
caller does something with the result. Commit 5c8418cf4025
("PCI/pwrctrl: Unregister platform device only if one actually exists")
has plenty of explanation of what really goes wrong.

> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > It would be a bit more efficient to check this up front before we
> > iterate thru devices, but there's a number of users of these functions
> > and this isn't really a hot path.
> 
> Yeah, this should be fine.  Does this fix a problem now and we need it
> merged for 6.13-final and backported, or can it just wait for 6.14-rc1?

It's a preventive measure to help head off future confusing bugs. It
doesn't need expedited merging or backporting.

FWIW, last week, I also cooked this change locally (+ the ACPI change;
and a kunit test for added fun), before I noticed Rob submitted this
one. If you'd rather, I can submit my patch series. Or I can submit my
patch series on top of this. Whichever you'd prefer.

Brian

On Mon, Dec 9, 2024 at 5:47 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Greg,
>
> On Wed, Dec 04, 2024 at 10:37:06AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 03, 2024 at 06:02:59PM -0600, Rob Herring (Arm) wrote:
> > > It recently came up that of_find_device_by_node() will match a device
> > > with a NULL of_node pointer. This is not desired behavior. The returned
> > > struct device is also not deterministic.
> >
> > It's not deterministic because a NULL pointer will cause that to happen,
> > or for some other reason?
>
> It'll pick the first platform device with no of_node. That likely yields
> something very wrong, but doesn't produce a visible problem until a
> caller does something with the result. Commit 5c8418cf4025
> ("PCI/pwrctrl: Unregister platform device only if one actually exists")
> has plenty of explanation of what really goes wrong.
>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > > It would be a bit more efficient to check this up front before we
> > > iterate thru devices, but there's a number of users of these functions
> > > and this isn't really a hot path.
> >
> > Yeah, this should be fine.  Does this fix a problem now and we need it
> > merged for 6.13-final and backported, or can it just wait for 6.14-rc1?
>
> It's a preventive measure to help head off future confusing bugs. It
> doesn't need expedited merging or backporting.
>
> FWIW, last week, I also cooked this change locally (+ the ACPI change;
> and a kunit test for added fun), before I noticed Rob submitted this
> one. If you'd rather, I can submit my patch series. Or I can submit my
> patch series on top of this. Whichever you'd prefer.

If you have a kunit test, you win. :)

Rob

On Tue, Dec 10, 2024 at 06:33:05AM -0600, Rob Herring wrote:
> On Mon, Dec 9, 2024 at 5:47 PM Brian Norris <briannorris@chromium.org> wrote:
> > FWIW, last week, I also cooked this change locally (+ the ACPI change;
> > and a kunit test for added fun), before I noticed Rob submitted this
> > one. If you'd rather, I can submit my patch series. Or I can submit my
> > patch series on top of this. Whichever you'd prefer.
> 
> If you have a kunit test, you win. :)

Ha, OK:

https://lore.kernel.org/linux-kselftest/20241210191353.533801-1-briannorris@chromium.org/
Subject: [PATCH 0/4] drivers: base: Don't match device with NULL of_node/fwnode/etc + tests

(Side note: I just noticed my mail script managed to skip LKML, although
it got the acpi, kunit, and kselftest lists. I can resend if that's a
problem.)

Brian