From nobody Wed Nov 27 04:41:59 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=1559208919; cv=none; d=zoho.com; s=zohoarc; b=QNIKv/Td+g22YJrxQw/TDoghas7pMtcMyTQscQPdYd0LVqX4IrY1axJCdu96G89/1dNgK4NavL5sFXAF8ehvxbF7MTEUwkw0AiOcFgZTfzItE5dDopGWDWwTD69ZMpeAMzvklCJZ0LWwtGpWjHi+7QWgSvkwmx1ErrWgHIVbAjI= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com; s=zohoarc; t=1559208919; h=Content-Type:Content-Transfer-Encoding:Cc: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=TI+1JNCQ7GdMKhirH12VhbUDJEgAn6weamSEgHSEpYM=; b=Gn2EiSVg9bIY63DtFns7vyWjIgD93iOm71eserePKht1VC1zbKAMuZ5ZC/kmZ7U4cKkB357Vj8xQxzqI9TvtjoNrI93GH7qEkpbDMhyISMlKdYuZP16ZLmk2v9JhxHbdr7OSmaKU39uq3H6KbE4jONAvtepiSxJXK+2y8demHzw= 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 1559208919916324.4326020903816; Thu, 30 May 2019 02:35:19 -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 24785F9E75; Thu, 30 May 2019 09:35:18 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id EE1D16C35F; Thu, 30 May 2019 09:35:17 +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 A1CE51806B1A; Thu, 30 May 2019 09:35:17 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id x4U9YISH027514 for ; Thu, 30 May 2019 05:34:18 -0400 Received: by smtp.corp.redhat.com (Postfix) id 5DF4860C7E; Thu, 30 May 2019 09:34:18 +0000 (UTC) Received: from moe.brq.redhat.com (unknown [10.43.2.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id B651F7B001; Thu, 30 May 2019 09:34:15 +0000 (UTC) From: Michal Privoznik To: libvir-list@redhat.com Date: Thu, 30 May 2019 11:34:02 +0200 Message-Id: <6b6d5d944b7a257a964f144ec1de9aa6a98504d3.1559208506.git.mprivozn@redhat.com> In-Reply-To: References: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-loop: libvir-list@redhat.com Cc: nshirokovskiy@virtuozzo.com Subject: [libvirt] [PATCH 7/9] qemu: Grab modify job for changing domain XML 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.38]); Thu, 30 May 2019 09:35:18 +0000 (UTC) Content-Type: text/plain; charset="utf-8" Changing domain definition must be guarded with acquiring modify job. The problem is if there is a thread executing say qemuDomainSetMemoryStatsPeriod() which is an API that acquires modify job and then possibly unlock the domain object and locks it again. However, becasue virDomainObjAssignDef() does not take a job (only object lock) it may have changed the domain definition while the other thread unlocked the domain object in order to talk on the monitor. For instance: Thread1: 1) lookup domain object and lock it 2) acquire job 3) get pointers to live and persistent defs 4) unlock the domain object 5) talk to qemu on the monitor Thread2 (Execute qemuDomainDefineXMLFlags): 1) lookup domain object and lock it 2) virDomainObjAssignDef() which overwrites persistent def and frees the old one 3) unlock domain object Thread1: 6) lock the domain object again 7) try to access the persistent def via pointer stored in 3) Now this will crash because the pointer from step 3) points to a memory that was freed. However, if Thread2 waited and acquired modify job (just like Thread1) then these two would serialize and no harm would be done. Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++---- src/qemu/qemu_migration.c | 7 +++++++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c61d39b12b..fff2f1932e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8784,7 +8784,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, return; } =20 - qemuDomainRemoveInactiveCommon(driver, vm); + if (vm->def) + qemuDomainRemoveInactiveCommon(driver, vm); =20 virDomainObjListRemove(driver->domains, vm); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8bbac339e0..77cb7e4b87 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1706,9 +1706,16 @@ static virDomainPtr qemuDomainCreateXML(virConnectPt= r conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; =20 + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { + qemuDomainRemoveInactive(driver, vm); + goto cleanup; + } + virDomainObjAssignDef(vm, def, true, NULL); def =3D NULL; =20 + qemuDomainObjEndJob(driver, vm); + if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { qemuDomainRemoveInactiveJob(driver, vm); @@ -6976,6 +6983,11 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; =20 + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { + qemuDomainRemoveInactive(driver, vm); + goto cleanup; + } + virDomainObjAssignDef(vm, def, true, NULL); def =3D NULL; =20 @@ -6988,6 +7000,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, priv =3D vm->privateData; priv->hookRun =3D true; } + qemuDomainObjEndJob(driver, vm); =20 if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_RESTORE, flags) < 0) @@ -7651,6 +7664,11 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, driver->xmlopt, 0))) goto cleanup; =20 + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { + qemuDomainRemoveInactive(driver, vm); + goto cleanup; + } + virDomainObjAssignDef(vm, def, false, &oldDef); def =3D NULL; =20 @@ -7673,7 +7691,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, vm->persistent =3D 0; qemuDomainRemoveInactiveJob(driver, vm); } - goto cleanup; + goto endjob; } =20 event =3D virDomainEventLifecycleNewFromObj(vm, @@ -7685,6 +7703,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, VIR_INFO("Creating domain '%s'", vm->def->name); dom =3D virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); =20 + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: virDomainDefFree(oldDef); virDomainDefFree(def); @@ -16889,14 +16910,14 @@ static virDomainPtr qemuDomainQemuAttach(virConne= ctPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; =20 - virDomainObjAssignDef(vm, def, true, NULL); - def =3D NULL; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { qemuDomainRemoveInactive(driver, vm); goto cleanup; } =20 + virDomainObjAssignDef(vm, def, true, NULL); + def =3D NULL; + if (qemuProcessAttach(conn, driver, vm, pid, pidfile, monConfig, monJSON) < 0) { qemuDomainRemoveInactive(driver, vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9e19c923ee..32325c9588 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2408,9 +2408,16 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; =20 + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { + qemuDomainRemoveInactive(driver, vm); + goto cleanup; + } + virDomainObjAssignDef(vm, *def, true, NULL); *def =3D NULL; =20 + qemuDomainObjEndJob(driver, vm); + priv =3D vm->privateData; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; --=20 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list