[libvirt] [PATCH] Correct two memory leaks triggered by udev events
Hi List, Please find below a patch that should correct two memory leaks within the udev device handling code. The issue is triggered by 'add' udev calls and will slowly cause libvirtd to consume the majority of system memory. The first part of the patch deals with udevAddOneDevice() and frees the memory allocated to 'def' if the function would return -1 (an error condition) due to the inability to look up udev properties (for example, if the udev device is already removed). The second part of the patch deals with a remaining 8kB/cycle memory leak, in which the udev device reference isn't released at the end of running udevEventHandleCallback(). I'm happy to answer any questions about the patch, there is also a bit more background at https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/571093 ---
From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00 2001 From: Nigel Jones <dev@nigelj.com> Date: Mon, 24 May 2010 15:05:53 +0000 Subject: [PATCH] Patch 2 memory leaks.
1. Ensure that memory is free'd from udevAddOneDevice() if the return value will be non-zero 2. Release udev device reference in udevEventHandleCallback() similar to the release of the reference in udevProcessDeviceListEntry() --- src/node_device/node_device_udev.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a1ced87..4d0effa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device) ret = 0; out: + if (ret != 0) { + virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */ + } return ret; } @@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, } out: + udev_device_unref(device); return; } -- 1.7.0.4
On 05/24/2010 12:10 PM, Nigel Jones wrote:
From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00 2001 From: Nigel Jones <dev@nigelj.com> Date: Mon, 24 May 2010 15:05:53 +0000 Subject: [PATCH] Patch 2 memory leaks.
1. Ensure that memory is free'd from udevAddOneDevice() if the return value will be non-zero 2. Release udev device reference in udevEventHandleCallback() similar to the release of the reference in udevProcessDeviceListEntry() --- src/node_device/node_device_udev.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a1ced87..4d0effa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device) ret = 0;
out: + if (ret != 0) { + virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */ + } return ret; }
@@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, }
out: + udev_device_unref(device); return; }
Good catch on both of these. I had to convince myself that udev_device_unref() would handle a NULL device (in the case of error before we allocated device), but it does. ACK -- Chris Lalancette
On 05/24/2010 12:32 PM, Chris Lalancette wrote:
On 05/24/2010 12:10 PM, Nigel Jones wrote:
From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00 2001 From: Nigel Jones <dev@nigelj.com> Date: Mon, 24 May 2010 15:05:53 +0000 Subject: [PATCH] Patch 2 memory leaks.
1. Ensure that memory is free'd from udevAddOneDevice() if the return value will be non-zero 2. Release udev device reference in udevEventHandleCallback() similar to the release of the reference in udevProcessDeviceListEntry() --- src/node_device/node_device_udev.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a1ced87..4d0effa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device) ret = 0;
out: + if (ret != 0) { + virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */ + } return ret; }
@@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, }
out: + udev_device_unref(device); return; }
Good catch on both of these. I had to convince myself that udev_device_unref() would handle a NULL device (in the case of error before we allocated device), but it does.
OK, so after talking to David Allan, who originally wrote this code, it seems like this is right approach, but there is one piece left that we have to worry about. Namely, on a udev "change" event, we have to make sure we remove and re-add the device to the node-device list. Now, this *may* already be happening, but I don't quite understand the semantics of this enough to say for sure. I'm going to hold off pushing this into the repo until Dave has had a chance to look at it and comment. -- Chris Lalancette
On Tue, May 25, 2010 at 04:10:27AM +1200, Nigel Jones wrote:
Hi List,
Please find below a patch that should correct two memory leaks within the udev device handling code.
The issue is triggered by 'add' udev calls and will slowly cause libvirtd to consume the majority of system memory.
The first part of the patch deals with udevAddOneDevice() and frees the memory allocated to 'def' if the function would return -1 (an error condition) due to the inability to look up udev properties (for example, if the udev device is already removed).
The second part of the patch deals with a remaining 8kB/cycle memory leak, in which the udev device reference isn't released at the end of running udevEventHandleCallback().
I'm happy to answer any questions about the patch, there is also a bit more background at https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/571093
Hi Nigel, Thanks for reporting this, and the patch. While reviewing your patch, I found an additional leak in the remove case which I have also addressed in the attached patch, which is otherwise just a slight modification of yours. I was able to reproduce the leaks by running a device add/remove cycle, and I do not see memory leakage after applying this patch. Let me know if it fixes it for you as well. Dave
On 05/28/10 - 11:13:29PM, Dave Allan wrote:
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6e3ecd7..5193f5b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1211,6 +1211,8 @@ static int udevRemoveOneDevice(struct udev_device *device) } nodeDeviceUnlock(driverState);
+ udev_device_unref(device); + return ret; }
Instead of adding the udev_device_unref here and in in udevAddOneDevice, I think we should probably just put it in the udevEventHandleCallback() and udevProcessDeviceListEntry(). Besides the fact that it is easier to read and verify that you've dropped the reference, if udev ever grows new actions besides "add", "change", or "remove", we'll go back to leaking the reference. -- Chris Lalancette
Here's a new version of the fix for the udev memory leak based on Chris Lalancette's feedback. I've tested it with valgrind as well as through an extremely long cycle of device additions and removals, so I'm confident in it, but I'd appreciate an ack that it fixes the reported problem. Dave David Allan (1): Fix leaks in udev device add/remove v3 src/node_device/node_device_udev.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)
* This patch is a modification of a patch submitted by Nigel Jones. It fixes several memory leaks on device addition/removal: 1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return value is non-zero 2. Always release the node device reference after the device has been processed. * Refactored for better readability per the suggestion of clalance --- src/node_device/node_device_udev.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6e3ecd7..73217c5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1309,13 +1309,14 @@ static int udevAddOneDevice(struct udev_device *device) goto out; } + /* If this is a device change, the old definition will be freed + * and the current definition will take its place. */ nodeDeviceLock(driverState); dev = virNodeDeviceAssignDef(&driverState->devs, def); nodeDeviceUnlock(driverState); if (dev == NULL) { VIR_ERROR(_("Failed to create device for '%s'"), def->name); - virNodeDeviceDefFree(def); goto out; } @@ -1324,6 +1325,10 @@ static int udevAddOneDevice(struct udev_device *device) ret = 0; out: + if (ret != 0) { + virNodeDeviceDefFree(def); + } + return ret; } @@ -1338,15 +1343,17 @@ static int udevProcessDeviceListEntry(struct udev *udev, name = udev_list_entry_get_name(list_entry); device = udev_device_new_from_syspath(udev, name); + if (device != NULL) { if (udevAddOneDevice(device) != 0) { VIR_INFO("Failed to create node device for udev device '%s'", name); } - udev_device_unref(device); ret = 0; } + udev_device_unref(device); + return ret; } @@ -1454,6 +1461,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, } out: + udev_device_unref(device); return; } -- 1.7.0.1
On 06/07/2010 11:23 AM, David Allan wrote:
* This patch is a modification of a patch submitted by Nigel Jones. It fixes several memory leaks on device addition/removal:
1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return value is non-zero
2. Always release the node device reference after the device has been processed.
* Refactored for better readability per the suggestion of clalance
Yes, in comparing the two versions, I can see the improvement. ACK on this version. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jun 08, 2010 at 12:38:47PM -0600, Eric Blake wrote:
On 06/07/2010 11:23 AM, David Allan wrote:
* This patch is a modification of a patch submitted by Nigel Jones. It fixes several memory leaks on device addition/removal:
1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return value is non-zero
2. Always release the node device reference after the device has been processed.
* Refactored for better readability per the suggestion of clalance
Yes, in comparing the two versions, I can see the improvement. ACK on this version.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Thanks, pushed. Dave
participants (5)
-
Chris Lalancette -
Dave Allan -
David Allan
-
Eric Blake -
Nigel Jones