| Message ID | 20241217035413.2892000-1-make_ruc2021@163.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: fix reference leak in pci_alloc_child_bus() | expand |
On Tue, 17 Dec 2024, Ma Ke wrote: > When device_register(&child->dev) failed, calling put_device() to > explicitly release child->dev. Otherwise, it could cause double free > problem. > > Found by code review. > > Cc: stable@vger.kernel.org > Fixes: 4f535093cf8f ("PCI: Put pci_dev in device tree as early as possible") > Signed-off-by: Ma Ke <make_ruc2021@163.com> > --- > drivers/pci/probe.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2e81ab0f5a25..d3146c588d7f 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1174,7 +1174,10 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > add_dev: > pci_set_bus_msi_domain(child); > ret = device_register(&child->dev); > - WARN_ON(ret < 0); > + if (ret) { > + WARN_ON(ret < 0); The usual way is: if (WARN_ON(ret < 0)) > + put_device(&child->dev); > + } > > pcibios_add_bus(child); But more serious problem here is that should this code even proceed as if nothing happened when an error occurs? pci_register_host_bridge() does proper rollback when device_register() fails but this function doesn't. Into the same vein, is using WARN_ON() even correct here? Why should this print a stacktrace if device_register() fails instead of simply printing and error?
Ilpo Järvinen<ilpo.jarvinen@linux.intel.com> wrote: > Ma Ke <make_ruc2021@163.com> writes: > > When device_register(&child->dev) failed, calling put_device() to > > explicitly release child->dev. Otherwise, it could cause double free > > problem. > > > > Found by code review. > > > > Cc: stable@vger.kernel.org > > Fixes: 4f535093cf8f ("PCI: Put pci_dev in device tree as early as possible") > > Signed-off-by: Ma Ke <make_ruc2021@163.com> > > --- > > drivers/pci/probe.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 2e81ab0f5a25..d3146c588d7f 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1174,7 +1174,10 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > > add_dev: > > pci_set_bus_msi_domain(child); > > ret = device_register(&child->dev); > > - WARN_ON(ret < 0); > > + if (ret) { > > + WARN_ON(ret < 0); > > The usual way is: > > if (WARN_ON(ret < 0)) > > > + put_device(&child->dev); > > + } > > > > pcibios_add_bus(child); > > But more serious problem here is that should this code even proceed as if > nothing happened when an error occurs? pci_register_host_bridge() does > proper rollback when device_register() fails but this function doesn't. > > Into the same vein, is using WARN_ON() even correct here? Why should this > print a stacktrace if device_register() fails instead of simply printing > and error? Thank you for your guidance and suggestions. I have the same confusion about the simple handling(WARN_ON()) of errors if device_add() fails. I am looking forward to receiving guidance and insights from other experts. -- Regards, Ma Ke
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2e81ab0f5a25..d3146c588d7f 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1174,7 +1174,10 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, add_dev: pci_set_bus_msi_domain(child); ret = device_register(&child->dev); - WARN_ON(ret < 0); + if (ret) { + WARN_ON(ret < 0); + put_device(&child->dev); + } pcibios_add_bus(child);
When device_register(&child->dev) failed, calling put_device() to explicitly release child->dev. Otherwise, it could cause double free problem. Found by code review. Cc: stable@vger.kernel.org Fixes: 4f535093cf8f ("PCI: Put pci_dev in device tree as early as possible") Signed-off-by: Ma Ke <make_ruc2021@163.com> --- drivers/pci/probe.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)