[libvirt] [PATCH v1 10/11] storage_driver: Protect pool def during startup and build

Ján Tomko jtomko at redhat.com
Wed Aug 21 12:03:42 UTC 2019


On Fri, May 24, 2019 at 04:35:46PM +0200, Michal Privoznik wrote:
>In near future the storage pool object lock will be released
>during startPool and buildPool callback (in some backends). But
>this means that another thread may acquire the pool object lock
>and change its definition rendering the former thread access not
>only stale definition but also access freed memory
>(virStoragePoolObjAssignDef() will free old def when setting a
>new one).
>
>One way out of this would be to have the pool appear as active
>because our code deals with obj->def and obj->newdef just fine.
>But we can't declare a pool as active if it's not started or
>still building up. Therefore, have a boolean flag that is very
>similar and forces virStoragePoolObjAssignDef() to store new
>definition in obj->newdef even for an inactive pool. In turn, we
>have to move the definition to correct place when unsetting the
>flag. But that's as easy as calling
>virStoragePoolUpdateInactive().
>
>Technically speaking, change made to
>storageDriverAutostartCallback() is not needed because until
>storage driver is initialized no storage API can run therefore
>there can't be anyone wanting to change the pool's definition.
>But I'm doing the change there for consistency anyways.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/conf/virstorageobj.c     | 26 +++++++++++++++++++++++++-
> src/conf/virstorageobj.h     |  6 ++++++
> src/libvirt_private.syms     |  2 ++
> src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++-
> 4 files changed, 63 insertions(+), 2 deletions(-)
>
>@@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>                                obj->def->name);
>                 goto cleanup;
>             }
>+
>+            if (virStoragePoolObjIsStarting(obj)) {
>+                virReportError(VIR_ERR_OPERATION_INVALID,
>+                               _("pool '%s' is starting up"),
>+                               obj->def->name);
>+                goto cleanup;
>+            }
>         }
>
>         VIR_STEAL_PTR(*objRet, obj);
>diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>index def4123b82..60bfa48e25 100644
>--- a/src/storage/storage_driver.c
>+++ b/src/storage/storage_driver.c
>@@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,

In the context not present in the mail there is:

>     if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef,
>                                          VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE |
>                                          VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE)))
>         goto cleanup;

which goes through the code path above in virStoragePoolObjIsDuplicate
that checks for starting storage pool,

>     newDef = NULL;
>     def = virStoragePoolObjGetDef(obj);
>
>+    virStoragePoolObjSetStarting(obj, true);
>+
>     if (backend->buildPool) {
>         if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
>             build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
>@@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn,
>     pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
>
>  cleanup:
>+    if (virStoragePoolObjIsStarting(obj)) {
>+        if (!virStoragePoolObjIsActive(obj))
>+            virStoragePoolUpdateInactive(obj);
>+        virStoragePoolObjSetStarting(obj, false);
>+    }
>     virObjectEventStateQueue(driver->storageEventState, event);
>     virStoragePoolObjEndAPI(&obj);
>     return pool;
>@@ -937,6 +953,8 @@ storagePoolCreate(virStoragePoolPtr pool,
>         goto cleanup;
>     }
>

however I don't see such a check here,

>+    virStoragePoolObjSetStarting(obj, true);
>+
>     if (backend->buildPool) {
>         if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
>             build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
>@@ -972,6 +990,11 @@ storagePoolCreate(virStoragePoolPtr pool,
>     ret = 0;
>
>  cleanup:
>+    if (virStoragePoolObjIsStarting(obj)) {
>+        if (!virStoragePoolObjIsActive(obj))
>+            virStoragePoolUpdateInactive(obj);
>+        virStoragePoolObjSetStarting(obj, false);
>+    }
>     virObjectEventStateQueue(driver->storageEventState, event);
>     virStoragePoolObjEndAPI(&obj);
>     return ret;
>@@ -1004,6 +1027,8 @@ storagePoolBuild(virStoragePoolPtr pool,
>         goto cleanup;
>     }
>

nor here.

>+    virStoragePoolObjSetStarting(obj, true);
>+
>     if (backend->buildPool &&
>         backend->buildPool(obj, flags) < 0)
>         goto cleanup;
>@@ -1016,6 +1041,10 @@ storagePoolBuild(virStoragePoolPtr pool,
>     ret = 0;
>
>  cleanup:
>+    if (virStoragePoolObjIsStarting(obj)) {
>+        virStoragePoolUpdateInactive(obj);
>+        virStoragePoolObjSetStarting(obj, false);
>+    }
>     virObjectEventStateQueue(driver->storageEventState, event);
>     virStoragePoolObjEndAPI(&obj);
>     return ret;

Also, storagePoolDelete, storagePoolUndefine and storagePoolDestroy
should be disallowed for pools that are starting.

With those added:
Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190821/d798d306/attachment-0001.sig>


More information about the libvir-list mailing list