[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