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@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(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index bdb167e9e2..9abcac479e 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -86,6 +86,7 @@ struct _virStoragePoolObj {
char *configFile;
char *autostartLink;
bool active;
+ bool starting;
bool autostart;
unsigned int asyncjobs;
@@ -312,6 +313,21 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
}
+void
+virStoragePoolObjSetStarting(virStoragePoolObjPtr obj,
+ bool starting)
+{
+ obj->starting = starting;
+}
+
+
+bool
+virStoragePoolObjIsStarting(virStoragePoolObjPtr obj)
+{
+ return obj->starting;
+}
+
+
bool
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj)
{
@@ -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);
@@ -1510,7 +1533,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj,
virStoragePoolDefPtr def,
unsigned int flags)
{
- if (virStoragePoolObjIsActive(obj)) {
+ if (virStoragePoolObjIsActive(obj) ||
+ virStoragePoolObjIsStarting(obj)) {
virStoragePoolDefFree(obj->newDef);
obj->newDef = def;
} else {
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index df699a84c5..7dfdf42b26 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -92,6 +92,12 @@ void
virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
bool active);
+void
+virStoragePoolObjSetStarting(virStoragePoolObjPtr obj,
+ bool starting);
+bool
+virStoragePoolObjIsStarting(virStoragePoolObjPtr obj);
+
bool
virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8fb366d218..243e3179cf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1196,6 +1196,7 @@ virStoragePoolObjGetVolumesCount;
virStoragePoolObjIncrAsyncjobs;
virStoragePoolObjIsActive;
virStoragePoolObjIsAutostart;
+virStoragePoolObjIsStarting;
virStoragePoolObjListAdd;
virStoragePoolObjListExport;
virStoragePoolObjListForEach;
@@ -1214,6 +1215,7 @@ virStoragePoolObjSetActive;
virStoragePoolObjSetAutostart;
virStoragePoolObjSetConfigFile;
virStoragePoolObjSetDef;
+virStoragePoolObjSetStarting;
virStoragePoolObjVolumeGetNames;
virStoragePoolObjVolumeListExport;
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
@@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
if (virStoragePoolObjIsAutostart(obj) &&
!virStoragePoolObjIsActive(obj)) {
+
+ virStoragePoolObjSetStarting(obj, true);
if (backend->startPool &&
backend->startPool(obj) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to autostart storage pool '%s': %s"),
def->name, virGetLastErrorMessage());
- return;
+ goto cleanup;
}
started = true;
}
@@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
virStoragePoolObjSetActive(obj, true);
}
}
+
+ cleanup:
+ if (virStoragePoolObjIsStarting(obj)) {
+ if (!virStoragePoolObjIsActive(obj))
+ virStoragePoolUpdateInactive(obj);
+ virStoragePoolObjSetStarting(obj, false);
+ }
}
@@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
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;
}
+ 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;
}
+ 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;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 5/24/19 10:35 AM, 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@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(-)
>
[...]
> 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
> @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
>
> if (virStoragePoolObjIsAutostart(obj) &&
> !virStoragePoolObjIsActive(obj)) {
> +
> + virStoragePoolObjSetStarting(obj, true);
> if (backend->startPool &&
> backend->startPool(obj) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to autostart storage pool '%s': %s"),
> def->name, virGetLastErrorMessage());
> - return;
> + goto cleanup;
> }
> started = true;
> }
> @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
> virStoragePoolObjSetActive(obj, true);
> }
> }
> +
> + cleanup:
> + if (virStoragePoolObjIsStarting(obj)) {
> + if (!virStoragePoolObjIsActive(obj))
> + virStoragePoolUpdateInactive(obj);
> + virStoragePoolObjSetStarting(obj, false);
> + }
> }
>
>
> @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
> newDef = NULL;
> def = virStoragePoolObjGetDef(obj);
>
Coverity let me know this morning that there's quite a few lines above
here which goto cleanup; however, cleanup expects @obj != NULL (or at
least virStoragePoolObjIsStarting does). Probably need similar logic
like you added in storagePool{Create|Build}.
John
> + 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;
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.