> -----Original Message-----
> From: Jan Kiszka [mailto:[email protected]]
> Sent: 2019年2月21日 0:29
> To: Flynn Xu <[email protected]>; [email protected]
> Cc: Henning Schild <[email protected]>
> Subject: Re: PCI driver memory leak
>
> On 20.02.19 09:08, Flynn Xu wrote:
> >
> >
> >> -----Original Message-----
> >> From: [email protected]
> >> [mailto:[email protected]] On Behalf Of Jan Kiszka
> >> Sent: 2019年2月20日 13:48
> >> To: Flynn Xu <[email protected]>; [email protected]
> >> Cc: Henning Schild <[email protected]>
> >> Subject: Re: PCI driver memory leak
> >>
> >> On 20.02.19 06:06, Flynn Xu wrote:
> >>> Hi,
> >>>
> >>> The function */pci_get_domain_bus_and_slot /*will increase the
> >>> reference count, and the caller must decrement the reference
> >>>
> >>> count by calling */pci_dev_put, /*so is there a potential memory leak?
> >>>
> >>> After add pci_dev_put, the counter decreased back to 0 when remove
> >>> pci
> >> devices..
> >>>
> >>> *//*
> >>>
> >>> diff --git a/driver/pci.c b/driver/pci.c
> >>>
> >>> index 8ba74bfd..7d060221 100644
> >>>
> >>> --- a/driver/pci.c
> >>>
> >>> +++ b/driver/pci.c
> >>>
> >>> @@ -103,8 +103,12 @@ static void jailhouse_pci_remove_device(const
> >>> struct jailhouse_pci_device *dev)
> >>>
> >>> l_dev = pci_get_domain_bus_and_slot(dev->domain,
> >>> PCI_BUS_NUM(dev->bdf),
> >>>
> >>> dev->bdf
> >> & 0xff);
> >>>
> >>> - if (l_dev)
> >>>
> >>> - pci_stop_and_remove_bus_device_locked(l_dev);
> >>>
> >>> + if (l_dev) {
> >>>
> >>> + pci_lock_rescan_remove();
> >>>
> >>> + pci_stop_and_remove_bus_device(l_dev);
> >>>
> >>> + pci_dev_put(l_dev);
> >>>
> >>> + pci_unlock_rescan_remove();
> >>>
> >>
> >> Good catch!
> >>
> >> But rather than the open-coded pci_lock_rescan_remove, isn't
> >> pci_stop_and_remove_bus_device_locked() the right thing? Or should we
> >> rather lock the whole procedure, including pci_get_domain_bus_and_slot?
> >> Please make sure to provide some reasoning for that when writing a patch.
> >
> > Sorry for not putting any reason before writing a patch, but I am not
> > sure what is the right way to handle this, I referenced how xen does
> > in pcifront_detach_devices in drivers/pci/xen-pcifront.c.
> >
>
> Yeah, I see. It remains clear why xen was patched this way. You find both
> pattern
> in the kernel, i.e. pci_dev_put inside the lock or outside. I think the
> outside
> variant is safe enough, though, and we can just use
> pci_stop_and_remove_bus_device_locked here.
>
Agree with you, I have tested with pci_dev_put outside of the lock, call
jailhouse
enable/disable thousands of time, and no race condition happen.
So do I need to send a patch for this?
Flynn
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence
> Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
