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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list