drivers/pci/probe.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9.
The newly added logic incorrectly sets bus_registered to true even when
device_register returns an error, this is incorrect.
When device_register fails, there is no need to release the reference count,
and there are no direct error-handling operations following its execution.
Therefore, this patch is meaningless and should be reverted.
Fixes: 804443c1f278 ("PCI: Fix reference leak in pci_register_host_bridge()")
Signed-off-by: Xiangwei Li <liwei728@huawei.com>
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
drivers/pci/probe.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 364fa2a514f8..8595d41add09 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -957,7 +957,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
resource_size_t offset, next_offset;
LIST_HEAD(resources);
struct resource *res, *next_res;
- bool bus_registered = false;
char addr[64], *fmt;
const char *name;
int err;
@@ -1021,7 +1020,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
name = dev_name(&bus->dev);
err = device_register(&bus->dev);
- bus_registered = true;
if (err)
goto unregister;
@@ -1110,15 +1108,12 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
unregister:
put_device(&bridge->dev);
device_del(&bridge->dev);
+
free:
#ifdef CONFIG_PCI_DOMAINS_GENERIC
pci_bus_release_domain_nr(parent, bus->domain_nr);
#endif
- if (bus_registered)
- put_device(&bus->dev);
- else
- kfree(bus);
-
+ kfree(bus);
return err;
}
--
2.25.1On 2025/4/10 11:28, Xiangwei Li wrote:
> This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9.
>
> The newly added logic incorrectly sets bus_registered to true even when
> device_register returns an error, this is incorrect.
>
> When device_register fails, there is no need to release the reference count,
I think you missed some thing about device_register(). This patch is wrong.
device_register()
-> device_initialize()
-> kobject_init()
-> kobject_init_internal()
-> kref_init(&kobj->kref); //set
kref->refcount to 1
^^^^^^^^^^^^^^^^^^^^^
device_register() only fails when device_add() fails, and
kerf->refcount can be 1
at this time , so we need to call put_deivce() to release memory.
> and there are no direct error-handling operations following its execution.
>
> Therefore, this patch is meaningless and should be reverted.
>
> Fixes: 804443c1f278 ("PCI: Fix reference leak in pci_register_host_bridge()")
> Signed-off-by: Xiangwei Li <liwei728@huawei.com>
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
在 2025/4/10 19:19, Su Hui 写道:
> On 2025/4/10 11:28, Xiangwei Li wrote:
>> This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9.
>>
>> The newly added logic incorrectly sets bus_registered to true even when
>> device_register returns an error, this is incorrect.
>>
>> When device_register fails, there is no need to release the reference
>> count,
> I think you missed some thing about device_register(). This patch is wrong.
>
> device_register()
> -> device_initialize()
> -> kobject_init()
> -> kobject_init_internal()
> -> kref_init(&kobj->kref); //set
> kref->refcount to 1
> ^^^^^^^^^^^^^^^^^^^^^
>
Sorry, I missed the initialization of refcount in device_initialize,
but I’m confused about the branch logic for bus_registered. Why isn’t
free(bus) executed when bus_registered == true? My understanding is
that the kobject_cleanup operation triggered when refcount reaches zero
does not clean up the allocated bus. Could you clarify this further?
Thanks,
Xiangwei Li
> device_register() only fails when device_add() fails, and
> kerf->refcount can be 1
> at this time , so we need to call put_deivce() to release memory.
>> and there are no direct error-handling operations following its
>> execution.
>>
>> Therefore, this patch is meaningless and should be reverted.
>>
>> Fixes: 804443c1f278 ("PCI: Fix reference leak in
>> pci_register_host_bridge()")
>> Signed-off-by: Xiangwei Li <liwei728@huawei.com>
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>
在 2025/4/12 11:18, liwei (JK) 写道:
>
>
> 在 2025/4/10 19:19, Su Hui 写道:
>> On 2025/4/10 11:28, Xiangwei Li wrote:
>>> This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9.
>>>
>>> The newly added logic incorrectly sets bus_registered to true even when
>>> device_register returns an error, this is incorrect.
>>>
>>> When device_register fails, there is no need to release the reference
>>> count,
>> I think you missed some thing about device_register(). This patch is
>> wrong.
>>
>> device_register()
>> -> device_initialize()
>> -> kobject_init()
>> -> kobject_init_internal()
>> -> kref_init(&kobj->kref); //set
>> kref->refcount to 1
>> ^^^^^^^^^^^^^^^^^^^^^
>>
> Sorry, I missed the initialization of refcount in device_initialize,
> but I’m confused about the branch logic for bus_registered. Why isn’t
> free(bus) executed when bus_registered == true? My understanding is
> that the kobject_cleanup operation triggered when refcount reaches zero
> does not clean up the allocated bus. Could you clarify this further?
>
> Thanks,
> Xiangwei Li
Hi Su,
Thank you very much for your explanation. Based on your response and an
analysis of the corresponding release paths, this confirms that the
issue I previously raised does not exist in your patch.
Release path:
kobject_cleanup
->device_release // device_ktype.device_release
->release_pcibus_dev // pcibus_class.release_pcibus_dev
->kfree(pci_bus) // free(bus)
Thanks,
Xiangwei Li
>> device_register() only fails when device_add() fails, and
>> kerf->refcount can be 1
>> at this time , so we need to call put_deivce() to release memory.
>>> and there are no direct error-handling operations following its
>>> execution.
>>>
>>> Therefore, this patch is meaningless and should be reverted.
>>>
>>> Fixes: 804443c1f278 ("PCI: Fix reference leak in
>>> pci_register_host_bridge()")
>>> Signed-off-by: Xiangwei Li <liwei728@huawei.com>
>>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>>
>
On 2025/4/12 11:18, liwei (JK) wrote: > 在 2025/4/10 19:19, Su Hui 写道: >> On 2025/4/10 11:28, Xiangwei Li wrote: >>> This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9. >>> >>> The newly added logic incorrectly sets bus_registered to true even when >>> device_register returns an error, this is incorrect. >>> >>> When device_register fails, there is no need to release the >>> reference count, >> I think you missed some thing about device_register(). This patch is >> wrong. >> >> device_register() >> -> device_initialize() >> -> kobject_init() >> -> kobject_init_internal() >> -> kref_init(&kobj->kref); //set >> kref->refcount to 1 >> ^^^^^^^^^^^^^^^^^^^^^ >> > Sorry, I missed the initialization of refcount in device_initialize, > but I’m confused about the branch logic for bus_registered. Why isn’t > free(bus) executed when bus_registered == true? My understanding is > that the kobject_cleanup operation triggered when refcount reaches zero > does not clean up the allocated bus. Could you clarify this further? > 1020 dev_set_name(&bus->dev, "%04x:%02x", pci_domain_nr(bus), bus->number); ^^^^^^^^^^^^^^^^^^^^ //device name is allocated, and should be freed when device_register() is failed. 1021 name = dev_name(&bus->dev); 1022 1023 err = device_register(&bus->dev); 1024 bus_registered = true; 1025 if (err) 1026 goto unregister; [...] 1117 if (bus_registered) 1118 put_device(&bus->dev); ^^^^^^^^^^^^^^^^ // decrement reference count to zero and call release_pcibus_dev() to free bus. // And call kfree_const() to free device name in kobject_cleanup(). 1119 else 1120 kfree(bus); Commit 804443c1f278 fixes the memory leak of 'name' and consistent with the annotation of device_degister(): '* NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead.' Su Hui
© 2016 - 2025 Red Hat, Inc.