[PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[Xiangwei Li]

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.1

Re: [PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[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
                            ^^^^^^^^^^^^^^^^^^^^^

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>

Re: [PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[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
> 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>
> 

Re: [PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[Su Hui]

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

Re: [PATCH] Revert "PCI: Fix reference leak in pci_register_host_bridge()"

[liwei (JK)]

在 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>
>>
>