Re: [PATCH 3/8] storage: Prior to creating a volume, refresh the pool

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02.10.2015 15:41, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1233003
> 
> Although perhaps bordering on a don't do that type scenario, if
> someone creates a volume in a pool outside of libvirt, then uses that
> same name to create a volume in the pool via libvirt, then the creation
> will fail and in some cases cause the same name volume to be deleted.
> 
> This patch will refresh the pool just prior to checking whether the
> named volume exists prior to creating the volume in the pool. While
> it's still possible to have a timing window to create a file after the
> check - at least we tried.  At that point, someone is being malicious.
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/storage/storage_driver.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 7aaa060..ddf4405 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1796,6 +1796,15 @@ storageVolCreateXML(virStoragePoolPtr obj,
>      if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0)
>          goto cleanup;
>  
> +    /* While not perfect, refresh the list of volumes in the pool and
> +     * then check that the incoming name isn't already in the pool.
> +     */
> +    if (backend->refreshPool) {
> +        virStoragePoolObjClearVols(pool);
> +        if (backend->refreshPool(obj->conn, pool) < 0)
> +            goto cleanup;
> +    }
> +
>      if (virStorageVolDefFindByName(pool, voldef->name)) {
>          virReportError(VIR_ERR_STORAGE_VOL_EXIST,
>                         _("'%s'"), voldef->name);
> 

Okay, this makes sense. Not only from the POV you are presenting, but I'd expect my pool to be refreshed after I create new volume in it.

And I think we have a way to eliminate even the little window - just track if we successfully built the volume or not. Something among these lines:

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ddf4405..dd28f3f 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1857,7 +1857,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
         voldef->building = true;
         virStoragePoolObjUnlock(pool);
 
-        buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags);
+        buildret = backend->buildVol(obj->conn, pool, buildvoldef, &created, flags);
 
         storageDriverLock();
         virStoragePoolObjLock(pool);
@@ -1866,7 +1866,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
         voldef->building = false;
         pool->asyncjobs--;
 
-        if (buildret < 0) {
+        if (buildret < 0 && created) {
             VIR_FREE(buildvoldef);
             storageVolDeleteInternal(volobj, backend, pool, voldef,
                                      0, false);

At any rate, ACK to this patch as is.

Michal

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]