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

[Su Hui <suhui@nfschina.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