From nobody Wed Nov 27 04:33:20 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=libvir-list-bounces@redhat.com; helo=mx1.redhat.com; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1558708685; cv=none; d=zoho.com; s=zohoarc; b=j+bArlKYr/1P2t53nKcaAmFWAv1pHNinX/6wa/bclcCwqtCqztGKFBGh6iOOt1EX5+2mKyuOoUfP0udYloKm+rl/OygrqW0XSlbZOYLy98h4EFNno1bLHrt0kOnty0fYVQ4miDtjwEDA9yksjAaVS67Fq9ZA3rVAlE1sTj3H1Rw= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com; s=zohoarc; t=1558708685; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To:ARC-Authentication-Results; bh=bLw5i13Yd8CrvvZ5VXUYpocen0Ub2BnpRmk7a7k0028=; b=jLHs6QSEGG0pL4Gk2qDJ10BGPYZQWwxWqw8OdrTFow2cvmraAxfBfcJkp+Ryr3Qg8YtTlLCT7lDs5pTxAJizNSf5y2V8JDzXf43qKU0mPXH3hg3oyjAajK20tGU5WBRlFvMDIvjGCKmilw5SgioyT0vJX2QN7Svk5NfYV05Rqo8= ARC-Authentication-Results: i=1; mx.zoho.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) header.from= Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.zohomail.com with SMTPS id 1558708685104955.3113121334225; Fri, 24 May 2019 07:38:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CD8913078AC8; Fri, 24 May 2019 14:37:59 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A66D57BE7D; Fri, 24 May 2019 14:37:59 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 653D95B427; Fri, 24 May 2019 14:37:58 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id x4OEaGPo011417 for ; Fri, 24 May 2019 10:36:16 -0400 Received: by smtp.corp.redhat.com (Postfix) id EA15769287; Fri, 24 May 2019 14:36:16 +0000 (UTC) Received: from moe.brq.redhat.com (unknown [10.43.2.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71C6D692A1 for ; Fri, 24 May 2019 14:36:09 +0000 (UTC) From: Michal Privoznik To: libvir-list@redhat.com Date: Fri, 24 May 2019 16:35:46 +0200 Message-Id: In-Reply-To: References: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-loop: libvir-list@redhat.com Subject: [libvirt] [PATCH v1 10/11] storage_driver: Protect pool def during startup and build X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Fri, 24 May 2019 14:38:03 +0000 (UTC) Content-Type: text/plain; charset="utf-8" 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 Reviewed-by: J=C3=A1n Tomko --- 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; =20 @@ -312,6 +313,21 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj, } =20 =20 +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting) +{ + obj->starting =3D starting; +} + + +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj) +{ + return obj->starting; +} + + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj) { @@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPt= r pools, obj->def->name); goto cleanup; } + + if (virStoragePoolObjIsStarting(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pool '%s' is starting up"), + obj->def->name); + goto cleanup; + } } =20 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 =3D 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); =20 +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting); +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj); + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj); =20 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; =20 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 o= bj, =20 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 =3D true; } @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr ob= j, virStoragePoolObjSetActive(obj, true); } } + + cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } } =20 =20 @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, newDef =3D NULL; def =3D virStoragePoolObjGetDef(obj); =20 + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |=3D VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn, pool =3D virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); =20 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; } =20 + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |=3D VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -972,6 +990,11 @@ storagePoolCreate(virStoragePoolPtr pool, ret =3D 0; =20 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; } =20 + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool && backend->buildPool(obj, flags) < 0) goto cleanup; @@ -1016,6 +1041,10 @@ storagePoolBuild(virStoragePoolPtr pool, ret =3D 0; =20 cleanup: + if (virStoragePoolObjIsStarting(obj)) { + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; --=20 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list