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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list