[libvirt] [PATCH] nodedev: Fix double unlock of the driver on udevEnumerateDevices failure
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4762f1969..4c35fd5c9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1789,18 +1789,18 @@ nodeStateInitialize(bool privileged, nodeDeviceLock(); if (!(driver->devs = virNodeDeviceObjListNew())) - goto cleanup; + goto unlock; driver->nodeDeviceEventState = virObjectEventStateNew(); if (udevPCITranslateInit(privileged) < 0) - goto cleanup; + goto unlock; udev = udev_new(); if (!udev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create udev context")); - goto cleanup; + goto unlock; } #if HAVE_UDEV_LOGGING /* cast to get rid of missing-format-attribute warning */ @@ -1811,7 +1811,7 @@ nodeStateInitialize(bool privileged, if (priv->udev_monitor == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_new_from_netlink returned NULL")); - goto cleanup; + goto unlock; } udev_monitor_enable_receiving(priv->udev_monitor); @@ -1837,11 +1837,11 @@ nodeStateInitialize(bool privileged, VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, NULL, NULL); if (priv->watch == -1) - goto cleanup; + goto unlock; /* Create a fictional 'computer' device to root the device tree. */ if (udevSetupSystemDev() != 0) - goto cleanup; + goto unlock; nodeDeviceUnlock(); @@ -1852,9 +1852,12 @@ nodeStateInitialize(bool privileged, return 0; cleanup: - nodeDeviceUnlock(); nodeStateCleanup(); return -1; + + unlock: + nodeDeviceUnlock(); + goto cleanup; } -- 2.13.3
On Wed, Jul 26, 2017 at 10:45:11AM +0200, Erik Skultety wrote:
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
The same issue still exists in the hal driver even after this patch. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On 07/26/2017 04:45 AM, Erik Skultety wrote:
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
Oh yeah - missed that case... thanks. w/r/t: _hal from Martin's review - that's pre-existing and separable. Still in both cases you're in Initialization functions with an unlock of an unlocked resource with no error checking by the same thread on your way to a function that's about to destroy the mutex... and eventual libvirtd death. Tks - John
On Wed, Jul 26, 2017 at 08:36:04AM -0400, John Ferlan wrote:
On 07/26/2017 04:45 AM, Erik Skultety wrote:
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
Oh yeah - missed that case... thanks.
w/r/t: _hal from Martin's review - that's pre-existing and separable.
Still in both cases you're in Initialization functions with an unlock of an unlocked resource with no error checking by the same thread on your way to a function that's about to destroy the mutex... and eventual libvirtd death.
Sure, but behaviour of an unlock of an unlocked resource is undefined and we most probably want to terminate the daemon gracefully, or better said, deterministically. Erik
On 07/26/2017 10:45 AM, Erik Skultety wrote:
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
I know you already pushed this but what's the point of node driver lock anyway? There are only two places where the driver lock is used - init and cleanup. Moreover with the current code still racy, except not really beacuse init and cleanup will never be called concurrently. Therefore the lock can be dropped entirely. Michal
On Fri, Jul 28, 2017 at 11:31:27AM +0200, Michal Privoznik wrote:
On 07/26/2017 10:45 AM, Erik Skultety wrote:
Commit @4cb719b2dc moved the driver locks around since these have become unnecessary at spots where the code handles now self-lockable object list, but missed the possible double unlock if udevEnumerateDevices fails, because at that point the driver lock had been already dropped.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_udev.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
I know you already pushed this but what's the point of node driver lock anyway? There are only two places where the driver lock is used - init
To answer the question: to protect the access to driver's private data which are mutable. Once we introduce an event handler thread for udev events, you'll get concurrent access to the udev monitor. As you say, the driver's lock doesn't serve much purpose except for accessing the udev monitor. This wasn't discussed on a mailing list unfortunately, but I asked about the need to perform any kinds of sanity checks on the udev_monitor on IRC, because noone can change that one except ourselves - let's say we dropped the sanity checks, then there's basically no need to have the driver lock (except for maybe consistency among other drivers) at all. But I recall some other opinions in favour of keeping the sanity checks, don't remember the specifics though. Erik
and cleanup. Moreover with the current code still racy, except not really beacuse init and cleanup will never be called concurrently. Therefore the lock can be dropped entirely.
Michal
participants (4)
-
Erik Skultety -
John Ferlan -
Martin Kletzander -
Michal Privoznik