From: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Sent: Tuesday, March 26, 2019 10:09 AM
On Thu, Mar 21, 2019 at 12:12:03AM +0000, Dexuan Cui wrote:
From: Michael Kelley mikelley@microsoft.com Sent: Wednesday, March 20, 2019 2:38 PM
From: Dexuan Cui decui@microsoft.com
After a device is just created in new_pcichild_device(), hpdev->refs is set to 2 (i.e. the initial value of 1 plus the get_pcichild()).
When we hot remove the device from the host, in Linux VM we first call hv_pci_eject_device(), which increases hpdev->refs by get_pcichild() and then schedules a work of hv_eject_device_work(), so hpdev->refs becomes 3 (let's ignore the paired get/put_pcichild() in other places). But in hv_eject_device_work(), currently we only call put_pcichild() twice, meaning the 'hpdev' struct can't be freed in put_pcichild(). This patch adds one put_pcichild() to fix the memory leak.
BTW, the device can also be removed when we run "rmmod pci-hyperv". On
this
path (hv_pci_remove() -> hv_pci_bus_exit() -> hv_pci_devices_present()), hpdev->refs is 2, and we do correctly call put_pcichild() twice in pci_devices_present_work().
Exiting new_pcichild_device() with hpdev->refs set to 2 seems OK to me. There is the reference in the hbus->children list, and there is the reference that is returned to the caller.
So IMO the "normal" reference count should be 2. :-) IMO only when a hv_pci_dev device is about to be destroyed, its reference count can drop to less than 2, i.e. first temporarily drop to 1 (meaning the hv_pci_dev device is removed from hbus->children), and then drop to zero (meaning kfree(hpdev) is called).
But what is strange is that pci_devices_present_work() overwrites the reference returned in local variable hpdev without doing a put_pcichild().
I suppose you mean:
/* First, mark all existing children as reported missing. */ spin_lock_irqsave(&hbus->device_list_lock, flags); list_for_each_entry(hpdev, &hbus->children, list_entry) { hpdev->reported_missing = true; } spin_unlock_irqrestore(&hbus->device_list_lock, flags)This is not strange to me, because, in pci_devices_present_work(), at first we don't know which devices are about to disappear, so we pre-mark all devices to be potentially missing like that; if a device is still on the bus, we'll mark its hpdev->reported_missing to false later; only after we know exactly which devices are missing, we should call put_pcichild() against them. All these seem natural to me.
It seems like the "normal" reference count should be 1 when the child device is not being manipulated, not 2.
What does "not being manipulated" mean?
The fix would be to add a call to put_pcichild() when the return value from new_pcichild_device() is overwritten.
In pci_devices_present_work(), we NEVER "overwrite" the "hpdev" returned from new_pcichild_device(): the "reported_missing" field of the new hpdev is implicitly initialized to false in new_pcichild_device().
Then remove the call to put_pcichild() in pci_device_present_work() when missing children are moved to the local list. The children have been moved from one list to another, so there's no need to decrement the reference count. Then when everything in the local list is deleted, the reference is correctly decremented, presumably freeing the memory.
With this approach, the code in hv_eject_device_work() is correct. There's one call to put_pcichild() to reflect removing the child device from the hbus-> children list, and one call to put_pcichild() to pair with the get_pcichild() in hv_pci_eject_device().
Please refer to my replies above. IMO we should fix hv_eject_device_work() rather than pci_devices_present_work().
Have we reached a conclusion on this ? I would like to merge this series given that it is fixing bugs and it has hung in the balance for quite a while but it looks like Michael is not too happy about these patches and I need a maintainer ACK to merge them.
Thanks, Lorenzo
Dexuan and I have discussed the topic extensively offline. The patch works in its current form, and I'll agree to it.
Reviewed-by: Michael Kelley mikelley@microsoft.com