From nobody Mon Apr 29 01:57:00 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of redhat.com designates 205.139.110.120 as permitted sender) client-ip=205.139.110.120; envelope-from=libvir-list-bounces@redhat.com; helo=us-smtp-1.mimecast.com; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 205.139.110.120 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1584100877; cv=none; d=zohomail.com; s=zohoarc; b=SBSM5TZU4szAVUVxgxwZjA/z3K1C+pTXIESgRFiuMntSzqpKPCXVoH0O9gv+IKg89XZcZp35IUu71CVbYHtZH7GdRC9iaBR9o0C+kNnLzYM1q55RAyhFQXewOipmY684Pbk60ClOpfw2ctGy2n3wU2dZWgsfwcRluj1DBp8pa8Y= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1584100877; h=Content-Type:Content-Transfer-Encoding:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To; bh=WgllZqOWELs7h1BhkIqwcexkw4IfKcVOCxJa9dnm5rc=; b=Hlnw63hlEiidUhgWRd6qg0TKxMyL82T8+ES6SrybSkeBtlEYMRH53CkaNKCvi/sa0bJrp7+DiqGZ1x9Z0MB+TyxOjbmuZLLy3Cjs4k47GJ0Mc0isoTBoEmR28/AuhSWaalzWsdua4BKXrveA0B8qK6eygaaLI5BtgjS+zY4jpPA= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 205.139.110.120 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) header.from= Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by mx.zohomail.com with SMTPS id 1584100877214857.4789309276536; Fri, 13 Mar 2020 05:01:17 -0700 (PDT) Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-14-PHmO02ioOVOUloywE5lTjg-1; Fri, 13 Mar 2020 08:01:11 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5E38D19251CE; Fri, 13 Mar 2020 12:01:05 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1BAD719C6A; Fri, 13 Mar 2020 12:01:03 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id AD13D1832D67; Fri, 13 Mar 2020 12:01:00 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 02DC0wau032438 for ; Fri, 13 Mar 2020 08:00:58 -0400 Received: by smtp.corp.redhat.com (Postfix) id B349492D2A; Fri, 13 Mar 2020 12:00:58 +0000 (UTC) Received: from localhost.localdomain.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id AB59092D4A; Fri, 13 Mar 2020 12:00:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584100874; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=WgllZqOWELs7h1BhkIqwcexkw4IfKcVOCxJa9dnm5rc=; b=Zhu2sasIY/2TaxDysn9gV2bit8LtCGS6WF1Kupt4ioC7uWfKCftSMLuhH+X2DsfErAAviQ SpR6pqs7Tky8X1Bh30vzey4gzw/KqYf+KVjS4darPoiFPYteQGUEgD/aNPyeAL23Sn2Mq5 twAN4/wY/t43c3egQPadeymuJCNKbRI= X-MC-Unique: PHmO02ioOVOUloywE5lTjg-1 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= To: libvir-list@redhat.com Subject: [libvirt PATCH] nodedev: fix race in API usage vs initial device enumeration Date: Fri, 13 Mar 2020 12:00:35 +0000 Message-Id: <20200313120035.1180068-1-berrange@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-loop: libvir-list@redhat.com X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @redhat.com) Content-Type: text/plain; charset="utf-8" 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=C3=A9 Reviewed-by: Michal Privoznik --- 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(+) diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index c4d3c55d73..c9df8dedab 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -36,6 +36,8 @@ typedef struct _virNodeDeviceDriverState virNodeDeviceDri= verState; typedef virNodeDeviceDriverState *virNodeDeviceDriverStatePtr; struct _virNodeDeviceDriverState { virMutex lock; + virCond initCond; + bool initialized; =20 /* pid file FD, ensures two copies of the driver can't use the same ro= ot */ int lockFD; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_de= vice_driver.c index da92a4cf94..ee175e1095 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -156,6 +156,22 @@ nodeDeviceUnlock(void) } =20 =20 +static int +nodeDeviceWaitInit(void) +{ + nodeDeviceLock(); + while (!driver->initialized) { + if (virCondWait(&driver->initCond, &driver->lock) < 0) { + virReportSystemError(errno, "%s", + _("failed to wait on condition")); + nodeDeviceUnlock(); + return -1; + } + } + nodeDeviceUnlock(); + return 0; +} + int nodeNumOfDevices(virConnectPtr conn, const char *cap, @@ -166,6 +182,9 @@ nodeNumOfDevices(virConnectPtr conn, =20 virCheckFlags(0, -1); =20 + if (nodeDeviceWaitInit() < 0) + return -1; + return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, virNodeNumOfDevicesCheckACL); } @@ -183,6 +202,9 @@ nodeListDevices(virConnectPtr conn, =20 virCheckFlags(0, -1); =20 + if (nodeDeviceWaitInit() < 0) + return -1; + return virNodeDeviceObjListGetNames(driver->devs, conn, virNodeListDevicesCheckACL, cap, names, maxnames); @@ -199,6 +221,9 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; =20 + if (nodeDeviceWaitInit() < 0) + return -1; + return virNodeDeviceObjListExport(conn, driver->devs, devices, virConnectListAllNodeDevicesCheckACL, flags); @@ -228,6 +253,9 @@ nodeDeviceLookupByName(virConnectPtr conn, virNodeDeviceDefPtr def; virNodeDevicePtr device =3D NULL; =20 + if (nodeDeviceWaitInit() < 0) + return NULL; + if (!(obj =3D nodeDeviceObjFindByName(name))) return NULL; def =3D virNodeDeviceObjGetDef(obj); @@ -256,6 +284,9 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, =20 virCheckFlags(0, NULL); =20 + if (nodeDeviceWaitInit() < 0) + return NULL; + if (!(obj =3D virNodeDeviceObjListFindSCSIHostByWWNs(driver->devs, wwnn, wwpn))) return NULL; @@ -470,6 +501,10 @@ nodeDeviceCreateXML(virConnectPtr conn, const char *virt_type =3D NULL; =20 virCheckFlags(0, NULL); + + if (nodeDeviceWaitInit() < 0) + return NULL; + virt_type =3D virConnectGetType(conn); =20 if (!(def =3D virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt= _type))) @@ -512,6 +547,9 @@ nodeDeviceDestroy(virNodeDevicePtr device) g_autofree char *wwpn =3D NULL; unsigned int parent_host; =20 + if (nodeDeviceWaitInit() < 0) + return -1; + if (!(obj =3D nodeDeviceObjFindByName(device->name))) return -1; def =3D virNodeDeviceObjGetDef(obj); @@ -564,6 +602,9 @@ nodeConnectNodeDeviceEventRegisterAny(virConnectPtr con= n, if (virConnectNodeDeviceEventRegisterAnyEnsureACL(conn) < 0) return -1; =20 + if (nodeDeviceWaitInit() < 0) + return -1; + if (virNodeDeviceEventStateRegisterID(conn, driver->nodeDeviceEventSta= te, device, eventID, callback, opaque, freecb, &callbackID) < 0) @@ -580,6 +621,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr c= onn, if (virConnectNodeDeviceEventDeregisterAnyEnsureACL(conn) < 0) return -1; =20 + if (nodeDeviceWaitInit() < 0) + return -1; + if (virObjectEventStateDeregisterID(conn, driver->nodeDeviceEventState, callbackID, true) < 0) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_devic= e_hal.c index a48b4ffcd1..653f54ceca 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -611,6 +611,15 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED, 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")); + virMutexDestroy(&driver->lock); + VIR_FREE(driver); + return VIR_DRV_STATE_INIT_ERROR; + } + nodeDeviceLock(); =20 if (privileged) { @@ -701,6 +710,11 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED, } VIR_FREE(udi); =20 + nodeDeviceLock(); + driver->initialized =3D true; + nodeDeviceUnlock(); + virCondBroadcast(&driver->initCond); + return VIR_DRV_STATE_INIT_COMPLETE; =20 failure: @@ -733,6 +747,7 @@ nodeStateCleanup(void) =20 VIR_FREE(driver->stateDir); nodeDeviceUnlock(); + virCondDestroy(&driver->initCond); virMutexDestroy(&driver->lock); VIR_FREE(driver); return 0; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_devi= ce_udev.c index 536cae6c30..87bdbc5a0b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1477,6 +1477,7 @@ nodeStateCleanup(void) virPidFileRelease(driver->stateDir, "driver", driver->lockFD); =20 VIR_FREE(driver->stateDir); + virCondDestroy(&driver->initCond); virMutexDestroy(&driver->lock); VIR_FREE(driver); =20 @@ -1744,6 +1745,11 @@ nodeStateInitializeEnumerate(void *opaque) if (udevEnumerateDevices(udev) !=3D 0) goto error; =20 + nodeDeviceLock(); + driver->initialized =3D true; + nodeDeviceUnlock(); + virCondBroadcast(&driver->initCond); + return; =20 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")); + virMutexDestroy(&driver->lock); + VIR_FREE(driver); + return VIR_DRV_STATE_INIT_ERROR; + } =20 driver->privileged =3D privileged; =20 --=20 2.24.1