[libvirt PATCH] nodedev: fix race in API usage vs initial device enumeration

[Michal Prívozník]

On 13. 3. 2020 13:00, Daniel P. Berrangé wrote:
> During startup the udev node device driver impl uses a background thread
> to populate the list of devices to avoid blocking the daemon startup
> entirely. There is no synchronization to the public APIs, so it is
> possible for an application to start calling APIs before the device
> initialization is complete.
> 
> This was not a problem in the old approach where libvirtd was started
> on boot, as initialization would easily complete before any APIs were
> called.
> 
> With the use of socket activation, however, APIs are invoked from the
> very moment the daemon starts. This is easily seen by doing a
> 
>   'virsh -c nodedev:///system list'
> 
> the first time it runs it will only show one or two devices. The second
> time it runs it will show all devices. The solution is to introduce a
> flag and condition variable for APIs to synchronize against before
> returning any data.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/conf/virnodedeviceobj.h          |  2 ++
>  src/node_device/node_device_driver.c | 44 ++++++++++++++++++++++++++++
>  src/node_device/node_device_hal.c    | 15 ++++++++++
>  src/node_device/node_device_udev.c   | 13 ++++++++
>  4 files changed, 74 insertions(+)
> 

For both drivers:

> @@ -1744,6 +1745,11 @@ nodeStateInitializeEnumerate(void *opaque)
>      if (udevEnumerateDevices(udev) != 0)
>          goto error;
>  
> +    nodeDeviceLock();
> +    driver->initialized = true;
> +    nodeDeviceUnlock();
> +    virCondBroadcast(&driver->initCond);

Should this broadcast be in the critical section? Just asking, for this
simple case it may be okay.

> +
>      return;
>  
>   error:
> @@ -1806,6 +1812,13 @@ nodeStateInitialize(bool privileged,
>          VIR_FREE(driver);
>          return VIR_DRV_STATE_INIT_ERROR;
>      }
> +    if (virCondInit(&driver->initCond) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to initialize condition variable"));

And perhaps, virReportSystemError() would be nicer.

> +        virMutexDestroy(&driver->lock);
> +        VIR_FREE(driver);
> +        return VIR_DRV_STATE_INIT_ERROR;
> +    }
>  
>      driver->privileged = privileged;
>  
> 

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal