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