* [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus()
@ 2022-08-25 12:27 Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge() Yang Yingliang
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Yang Yingliang @ 2022-08-25 12:27 UTC (permalink / raw)
To: linux-kernel, linux-pci; +Cc: bhelgaas
If device_add() fails in pci_register_host_bridge(), the brigde device will
be put once, and it will be put again in error path of pci_create_root_bus().
Move the put_device() from pci_create_root_bus() to pci_register_host_bridge()
to fix this problem. And use device_unregister() instead of del_device() and
put_device().
Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/pci/probe.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c5286b027f00..e500eb9d6468 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1027,7 +1027,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
unregister:
put_device(&bridge->dev);
- device_del(&bridge->dev);
+ device_unregister(&bridge->dev);
free:
kfree(bus);
@@ -3037,13 +3037,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
error = pci_register_host_bridge(bridge);
if (error < 0)
- goto err_out;
+ return NULL;
return bridge->bus;
-
-err_out:
- put_device(&bridge->dev);
- return NULL;
}
EXPORT_SYMBOL_GPL(pci_create_root_bus);
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge()
2022-08-25 12:27 [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Yang Yingliang
@ 2022-08-25 12:27 ` Yang Yingliang
2022-08-26 22:38 ` Bjorn Helgaas
2022-08-25 12:27 ` [PATCH -next 3/3] PCI: fix handle error case in pci_alloc_child_bus() Yang Yingliang
2022-08-26 21:14 ` [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Bjorn Helgaas
2 siblings, 1 reply; 7+ messages in thread
From: Yang Yingliang @ 2022-08-25 12:27 UTC (permalink / raw)
To: linux-kernel, linux-pci; +Cc: bhelgaas
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;
+ }
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH -next 3/3] PCI: fix handle error case in pci_alloc_child_bus()
2022-08-25 12:27 [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge() Yang Yingliang
@ 2022-08-25 12:27 ` Yang Yingliang
2022-08-26 21:14 ` [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Bjorn Helgaas
2 siblings, 0 replies; 7+ messages in thread
From: Yang Yingliang @ 2022-08-25 12:27 UTC (permalink / raw)
To: linux-kernel, linux-pci; +Cc: bhelgaas
Return NULL pointer if device_register() fails, and call put_device()
to free the memory of pci bus and device name.
Fixes: 4f535093cf8f ("PCI: Put pci_dev in device tree as early as possible")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.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 292d9da146ce..c924f4e1ed38 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1144,7 +1144,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 (WARN_ON(ret < 0)) {
+ put_device(&child->dev);
+ return NULL;
+ }
pcibios_add_bus(child);
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus()
2022-08-25 12:27 [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge() Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 3/3] PCI: fix handle error case in pci_alloc_child_bus() Yang Yingliang
@ 2022-08-26 21:14 ` Bjorn Helgaas
2022-08-27 9:15 ` Yang Yingliang
2 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2022-08-26 21:14 UTC (permalink / raw)
To: Yang Yingliang; +Cc: linux-kernel, linux-pci, bhelgaas, Rob Herring
[+cc Rob]
On Thu, Aug 25, 2022 at 08:27:51PM +0800, Yang Yingliang wrote:
> If device_add() fails in pci_register_host_bridge(), the brigde device will
> be put once, and it will be put again in error path of pci_create_root_bus().
> Move the put_device() from pci_create_root_bus() to pci_register_host_bridge()
> to fix this problem. And use device_unregister() instead of del_device() and
> put_device().
s/brigde/bridge/
> Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")
If you're fixing a commit from somebody else, please always cc: the
author because the author can help review the change.
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> drivers/pci/probe.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c5286b027f00..e500eb9d6468 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1027,7 +1027,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>
> unregister:
> put_device(&bridge->dev);
> - device_del(&bridge->dev);
> + device_unregister(&bridge->dev);
I don't understand this part. device_unregister() looks like this:
void device_unregister(struct device *dev)
{
device_del(dev);
put_device(dev);
}
So this calls put_device(&bridge->dev) twice, doesn't it?
The "unregister" label looks poorly named. We only get there if
device_register() *failed*. We shouldn't need to unregister anything
in that case.
> free:
> kfree(bus);
> @@ -3037,13 +3037,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>
> error = pci_register_host_bridge(bridge);
> if (error < 0)
> - goto err_out;
> + return NULL;
>
> return bridge->bus;
> -
> -err_out:
> - put_device(&bridge->dev);
> - return NULL;
This part looks right to me. The get_device() is in
pci_register_host_bridge(), and if pci_register_host_bridge() returns
failure, I think it should first do the corresponding put_device().
> }
> EXPORT_SYMBOL_GPL(pci_create_root_bus);
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge()
2022-08-25 12:27 ` [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge() Yang Yingliang
@ 2022-08-26 22:38 ` Bjorn Helgaas
2022-08-27 9:29 ` Yang Yingliang
0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2022-08-26 22:38 UTC (permalink / raw)
To: Yang Yingliang
Cc: linux-kernel, linux-pci, bhelgaas, Arnd Bergmann, Rob Herring
[+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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus()
2022-08-26 21:14 ` [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Bjorn Helgaas
@ 2022-08-27 9:15 ` Yang Yingliang
0 siblings, 0 replies; 7+ messages in thread
From: Yang Yingliang @ 2022-08-27 9:15 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel, linux-pci, bhelgaas, Rob Herring
Hi,
On 2022/8/27 5:14, Bjorn Helgaas wrote:
> [+cc Rob]
>
> On Thu, Aug 25, 2022 at 08:27:51PM +0800, Yang Yingliang wrote:
>> If device_add() fails in pci_register_host_bridge(), the brigde device will
>> be put once, and it will be put again in error path of pci_create_root_bus().
>> Move the put_device() from pci_create_root_bus() to pci_register_host_bridge()
>> to fix this problem. And use device_unregister() instead of del_device() and
>> put_device().
> s/brigde/bridge/
>
>> Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")
> If you're fixing a commit from somebody else, please always cc: the
> author because the author can help review the change.
OK.
>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> drivers/pci/probe.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index c5286b027f00..e500eb9d6468 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1027,7 +1027,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>>
>> unregister:
>> put_device(&bridge->dev);
>> - device_del(&bridge->dev);
>> + device_unregister(&bridge->dev);
> I don't understand this part. device_unregister() looks like this:
>
> void device_unregister(struct device *dev)
> {
> device_del(dev);
> put_device(dev);
> }
>
> So this calls put_device(&bridge->dev) twice, doesn't it?
>
> The "unregister" label looks poorly named. We only get there if
> device_register() *failed*. We shouldn't need to unregister anything
> in that case.
If it goes to the 'unregister' label, the bridge->dev has been register
sucessfully (device_initialize() called from pci_alloc_host_bridge()
and device_add() called from pci_register_host_bridge()), so it need be
unregister, and another put_device() is for decreasing
refcount of 'bus->bridge'.
Thanks,
Yang
>
>> free:
>> kfree(bus);
>> @@ -3037,13 +3037,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>
>> error = pci_register_host_bridge(bridge);
>> if (error < 0)
>> - goto err_out;
>> + return NULL;
>>
>> return bridge->bus;
>> -
>> -err_out:
>> - put_device(&bridge->dev);
>> - return NULL;
> This part looks right to me. The get_device() is in
> pci_register_host_bridge(), and if pci_register_host_bridge() returns
> failure, I think it should first do the corresponding put_device().
>
>> }
>> EXPORT_SYMBOL_GPL(pci_create_root_bus);
>>
>> --
>> 2.25.1
>>
> .
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge()
2022-08-26 22:38 ` Bjorn Helgaas
@ 2022-08-27 9:29 ` Yang Yingliang
0 siblings, 0 replies; 7+ messages in thread
From: Yang Yingliang @ 2022-08-27 9:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, bhelgaas, Arnd Bergmann, Rob Herring
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
>>
> .
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-27 9:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-25 12:27 [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 2/3] PCI: fix possible memory leak in error case in pci_register_host_bridge() Yang Yingliang
2022-08-26 22:38 ` Bjorn Helgaas
2022-08-27 9:29 ` Yang Yingliang
2022-08-25 12:27 ` [PATCH -next 3/3] PCI: fix handle error case in pci_alloc_child_bus() Yang Yingliang
2022-08-26 21:14 ` [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus() Bjorn Helgaas
2022-08-27 9:15 ` Yang Yingliang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).