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

[John Ferlan]

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 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(-)
> 

[...]

> 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;

[...]