On 21.02.19 03:07, Flynn Xu wrote:
-----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?
Yes, please. Strictly spoken, we actually have two topics: the refcount leak and
the locked stop_and_remove.
If you are unsure about how to structure them, have a look at the git history of
Jailhouse and patches sent to the list.
Thanks in advance,
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.
