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

[John Ferlan]

On 10/05/2015 07:28 AM, Michal Privoznik wrote:
> 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 at redhat.com>
>> ---
>>  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:
> 

Did a bit of digging on this today - suffice to say it'll be tricky...

 * For some backends, the createVol actually does the creation (disk,
logical, zfs) while for others the createVol is just checking existence
and generating a key (fs, rbd, sheepdog) and buildVol does the magic
creation deed.

 * For those that support buildVol, there's an "intertwined
relationship" with buildVolFrom. There's even one (disk) that has the
buildVolFrom, but no buildVol. One would 'assume' (hah!) that
buildVolFrom would have an already created volume into which the from
volume gets moved, but I know what happens when one assumes...

 * There's some fragility related to NFS root squash and the
virFileOpenAs API (including the qemuOpenFileAs).

In any case, I've created some patches, but need a bit more digging time
to feel a bit more comfortable about them...

John

Initially in my mind the timing window was only "externally" based...

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