Re: [PATCH] cxl: Fix unbalanced pci_dev_get in cxl_probe

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 3336 bytes --]

Ahaha so I was wrong, device_add does grab a reference. 

> Currently, cxl_probe(pdev):
>  1) calls pci_dev_get(pdev)
>  2) calls cxl_adapter_init
>     a) init calls cxl_adapter_alloc, which creates a struct cxl, 
>        conventionally called adapter. This struct contains a 
>        device entry, adapter->dev.
> 
>     b) init calls cxl_configure_adapter, where we set 
>        adapter->dev.parent = &dev->dev (here dev is the pci dev)
> 
> So at this point, the cxl adapter's device's parent is the pci device
> that I want to be refcounted.
> 
>     c) init calls cxl_register_adapter (which inexplicably is in file.c)
> 
>        *) cxl_register_adapter calls device_register(&adapter->dev) 
> 
> So now we're in device_register, where dev is the adapter device, and we
> want to know if the PCI device is safe after we return.
> 
> device_register(&adapter->dev) calls device_initialize() and then
> device_add().
> 
> device_add() does a get_device(). That ends up being a kobject_get() on
> the adapter device kobj, which will increment the kref on the adapter
> device. 

I was right up to this point, but I didn't read enough of device_add.

device_add explicitly grabs the device's parent, and calls get_device on
it:

         parent = get_device(dev->parent);

So it turns out we *are* protected against the device disappearing, my
patch is correct and I don't need a v2.

Thanks to Ian for making me recheck device_add :)

Regards,
Daniel

> The PCI device doesn't get it's kref incremented explicitly.
> 
> It looks like we're not protected against that disappearing randomly.
> 
> So thanks, mpe, that was a good catch. I'll do a v2 that keeps the get
> and adds a matching put.
> 
> Regards,
> Daniel
> 
> On Wed, 2015-09-09 at 15:17 +1000, Daniel Axtens wrote:
> > Currently the first thing we do in cxl_probe is to grab a reference
> > on the pci device. Later on, we call device_register on our adapter,
> > which also holds the PCI device.
> > 
> > In our remove path, we call device_unregister, but we never call
> > pci_dev_put. We therefore leak the device every time we do a
> > reflash.
> > 
> > device_register/unregister is sufficient to hold the reference.
> > Drop the call to pci_dev_get.
> > 
> > Fixes: f204e0b8cedd ("cxl: Driver code for powernv PCIe based cards for userspace access")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > 
> > ---
> > 
> > This is the cxl bug that caused me to catch this a few weeks back:
> > e642d11bdbfe ("powerpc/eeh: Probe after unbalanced kref check")
> > 
> > I put an printk in the unbalanced kref path and confirmed that it
> > was printed with the pci_dev_get in and went away with the
> > pci_dev_get out.
> > ---
> >  drivers/misc/cxl/pci.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> > index 02c85160bfe9..a5e977192b61 100644
> > --- a/drivers/misc/cxl/pci.c
> > +++ b/drivers/misc/cxl/pci.c
> > @@ -1249,8 +1249,6 @@ static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  	int slice;
> >  	int rc;
> >  
> > -	pci_dev_get(dev);
> > -
> >  	if (cxl_verbose)
> >  		dump_cxl_config_space(dev);
> >  
> 

-- 
Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]