Message ID | 20220825122753.1838930-2-yangyingliang@huawei.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Bjorn Helgaas |
Headers | show |
Series | [-next,1/3] PCI: fix double put_device() in error case in pci_create_root_bus() | expand |
[+cc Arnd, Rob] On Thu, Aug 25, 2022 at 08:27:52PM +0800, Yang Yingliang wrote: > If device_register() fails in pci_register_host_bridge(), the refcount > of bus device is leaked, so device name that set by dev_set_name() can > not be freed. Fix this by calling put_device() when device_register() > fails, so the device name will be freed in kobject_cleanup(). > > Fixes: 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/pci/probe.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index e500eb9d6468..292d9da146ce 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -948,8 +948,17 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > name = dev_name(&bus->dev); > > err = device_register(&bus->dev); > - if (err) > - goto unregister; > + if (err) { > + /* > + * release_pcibus_dev() will decrease the refcount of bridge > + * device and free the memory of bus. > + * The memory of bus device name will be freed when the refcount > + * get to zero. > + */ > + put_device(&bus->dev); > + device_unregister(&bridge->dev); > + return err; > + } Calling put_device(X) after device_register(X) returns failure doesn't need explanation because that's the standard pattern. I think that was just missing before. In this error case, we previously did called put_device() for the *bridge* instead of the bus. That was likely a typo and seems like the important thing here. > pcibios_add_bus(bus); > > @@ -1025,10 +1034,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > > return 0; > > -unregister: > - put_device(&bridge->dev); > - device_unregister(&bridge->dev); > - > free: > kfree(bus); > return err; > -- > 2.25.1 >
On 2022/8/27 6:38, Bjorn Helgaas wrote: > [+cc Arnd, Rob] > > On Thu, Aug 25, 2022 at 08:27:52PM +0800, Yang Yingliang wrote: >> If device_register() fails in pci_register_host_bridge(), the refcount >> of bus device is leaked, so device name that set by dev_set_name() can >> not be freed. Fix this by calling put_device() when device_register() >> fails, so the device name will be freed in kobject_cleanup(). >> >> Fixes: 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/pci/probe.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index e500eb9d6468..292d9da146ce 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -948,8 +948,17 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> name = dev_name(&bus->dev); >> >> err = device_register(&bus->dev); >> - if (err) >> - goto unregister; >> + if (err) { >> + /* >> + * release_pcibus_dev() will decrease the refcount of bridge >> + * device and free the memory of bus. >> + * The memory of bus device name will be freed when the refcount >> + * get to zero. >> + */ >> + put_device(&bus->dev); >> + device_unregister(&bridge->dev); >> + return err; >> + } > Calling put_device(X) after device_register(X) returns failure doesn't > need explanation because that's the standard pattern. I think that > was just missing before. > > In this error case, we previously did called put_device() for the > *bridge* instead of the bus. That was likely a typo and seems like > the important thing here. put_device() for the bridge will be called in the callback of put for the bus. So it doesn't call put bridge device here. Thanks, Yang >> pcibios_add_bus(bus); >> >> @@ -1025,10 +1034,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> >> return 0; >> >> -unregister: >> - put_device(&bridge->dev); >> - device_unregister(&bridge->dev); >> - >> free: >> kfree(bus); >> return err; >> -- >> 2.25.1 >> > .
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index e500eb9d6468..292d9da146ce 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -948,8 +948,17 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) name = dev_name(&bus->dev); err = device_register(&bus->dev); - if (err) - goto unregister; + if (err) { + /* + * release_pcibus_dev() will decrease the refcount of bridge + * device and free the memory of bus. + * The memory of bus device name will be freed when the refcount + * get to zero. + */ + put_device(&bus->dev); + device_unregister(&bridge->dev); + return err; + } pcibios_add_bus(bus); @@ -1025,10 +1034,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) return 0; -unregister: - put_device(&bridge->dev); - device_unregister(&bridge->dev); - free: kfree(bus); return err;
If device_register() fails in pci_register_host_bridge(), the refcount of bus device is leaked, so device name that set by dev_set_name() can not be freed. Fix this by calling put_device() when device_register() fails, so the device name will be freed in kobject_cleanup(). Fixes: 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/pci/probe.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)