| Message ID | 1519526762-136838-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote: > pci_register_host_bridge records bus->domain_nr from > pci_bus_find_domain_nr but not guarantee not to pass a NULL > struct device *parent to it which could be explained by the hint > of checkcing for parent device before calling set_dev_node(), > just lines after that. So of_pci_bus_find_domain_nr wisely check > the parent pointer at the very beginning, but forgot to check it > again when trying to get of_node from parent, which could causes > a NULL pointer dereference. Fix it by dumping the NULL pointer > address simply, if no parent available. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/pci/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index f6a4dd1..ef18c48 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5612,7 +5612,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent) > domain = pci_get_new_domain_nr(); > } else { > dev_err(parent, "Node %pOF has inconsistent \"linux,pci-domain\" property in DT\n", > - parent->of_node); > + parent ? parent->of_node : NULL); I really need to get rid of this function in its current form. In the interim, I think that printing NULL as faulting node gives no information whatsoever so this patch should be updated either by avoiding to print or better by demoting the dev_err() to a pr_err(), whatever works better for Bjorn. Thanks, Lorenzo > domain = -1; > } > > -- > 1.9.1 > >
Dear Lorenzo, On 2018/2/28 1:49, Lorenzo Pieralisi wrote: > On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote: > I really need to get rid of this function in its current form. In the > interim, I think that printing NULL as faulting node gives no > information whatsoever so this patch should be updated either by > avoiding to print or better by demoting the dev_err() to a pr_err(), > whatever works better for Bjorn. > Ok, that sounds reasonable to me, so which ways is preferred, Bjorn?
On Wed, Feb 28, 2018 at 09:42:39AM +0800, Shawn Lin wrote: > On 2018/2/28 1:49, Lorenzo Pieralisi wrote: > > On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote: > > I really need to get rid of this function in its current form. In the > > interim, I think that printing NULL as faulting node gives no > > information whatsoever so this patch should be updated either by > > avoiding to print or better by demoting the dev_err() to a pr_err(), > > whatever works better for Bjorn. > > Ok, that sounds reasonable to me, so which ways is preferred, Bjorn? If we get to the point of that dev_err(), I don't think we really know what to do, so we should always print *something* as a clue to the user because I think things are going to break if we use -1 as the domain -- not because -1 is such an invalid value per se, but it's an indication that the DT doesn't correctly describe the machine. We may make incorrect assumptions about which devices are in the same domain. So I'd make it a pr_err() for now. It would be good to include the %pOF when "parent" is not NULL. The changelog could be simply: If the "parent" pointer passed to of_pci_bus_find_domain_nr() is NULL, don't dereference it.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index f6a4dd1..ef18c48 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5612,7 +5612,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent) domain = pci_get_new_domain_nr(); } else { dev_err(parent, "Node %pOF has inconsistent \"linux,pci-domain\" property in DT\n", - parent->of_node); + parent ? parent->of_node : NULL); domain = -1; }
pci_register_host_bridge records bus->domain_nr from pci_bus_find_domain_nr but not guarantee not to pass a NULL struct device *parent to it which could be explained by the hint of checkcing for parent device before calling set_dev_node(), just lines after that. So of_pci_bus_find_domain_nr wisely check the parent pointer at the very beginning, but forgot to check it again when trying to get of_node from parent, which could causes a NULL pointer dereference. Fix it by dumping the NULL pointer address simply, if no parent available. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)