From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42403) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuIoS-0000Yh-H2 for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:30:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XuIoN-0006zR-Jg for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:30:08 -0500 Received: from mail-wg0-x231.google.com ([2a00:1450:400c:c00::231]:38509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuIoM-0006wt-W7 for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:30:03 -0500 Received: by mail-wg0-f49.google.com with SMTP id n12so227428wgh.22 for ; Fri, 28 Nov 2014 02:30:02 -0800 (PST) Date: Fri, 28 Nov 2014 10:29:59 +0000 From: Stefan Hajnoczi Message-ID: <20141128102959.GB11358@stefanha-thinkpad.redhat.com> References: <1415719671-16257-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NDin8bjvE/0mNLFQ" Content-Disposition: inline In-Reply-To: <1415719671-16257-1-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6] qcow2: Buffer L1 table in snapshot refcount update List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , Zhang Haoyu , qemu-devel@nongnu.org, Stefan Hajnoczi --NDin8bjvE/0mNLFQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 11, 2014 at 04:27:51PM +0100, Max Reitz wrote: > From: Zhang Haoyu >=20 > Buffer the active L1 table in qcow2_update_snapshot_refcount() in order > to prevent in-place conversion of the L1 table buffer in the > BDRVQcowState to big endian and back, which would lead to data > corruption if that buffer was accessed concurrently. This should not > happen but better being safe than sorry. >=20 > Signed-off-by: Zhang Haoyu > Signed-off-by: Max Reitz > --- > v6 for "snapshot: use local variable to bdrv_pwrite_sync L1 table" (I > changed the commit message wording to make it more clear what this patch > does and why we want it). >=20 > Changes in v6: > - Only copy the local buffer back into s->l1_table if we are indeed > accessing the local L1 table > - Use qemu_vfree() instead of g_free() > --- > block/qcow2-refcount.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) If there is a code path where the L1 table is accessed while qcow2_update_snapshot_refcount() is blocked, this patch does not fix the bug. It trades an L1 table entry corruption (due to endianness mismatch on little-endian hosts) for a race condition where a stale L1 table is accessed or L1 changes are overwritten when qcow2_update_snapshot_refcount() memcpys back to s->l1_table. Please identify the root cause and fix that. > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 9afdb40..c0c4a50 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -877,14 +877,18 @@ int qcow2_update_snapshot_refcount(BlockDriverState= *bs, > { > BDRVQcowState *s =3D bs->opaque; > uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2; > - bool l1_allocated =3D false; > + bool active_l1 =3D false; > int64_t old_offset, old_l2_offset; > int i, j, l1_modified =3D 0, nb_csectors, refcount; > int ret; > =20 > l2_table =3D NULL; > - l1_table =3D NULL; > l1_size2 =3D l1_size * sizeof(uint64_t); > + l1_table =3D qemu_try_blockalign(bs->file, l1_size2); > + if (l1_table =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto fail; > + } > =20 > s->cache_discards =3D true; > =20 > @@ -892,13 +896,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState = *bs, > * l1_table_offset when it is the current s->l1_table_offset! Be car= eful > * when changing this! */ > if (l1_table_offset !=3D s->l1_table_offset) { > - l1_table =3D g_try_malloc0(align_offset(l1_size2, 512)); > - if (l1_size2 && l1_table =3D=3D NULL) { > - ret =3D -ENOMEM; > - goto fail; > - } > - l1_allocated =3D true; > - > ret =3D bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2= ); > if (ret < 0) { > goto fail; > @@ -908,8 +905,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *= bs, > be64_to_cpus(&l1_table[i]); > } else { > assert(l1_size =3D=3D s->l1_size); > - l1_table =3D s->l1_table; > - l1_allocated =3D false; > + memcpy(l1_table, s->l1_table, l1_size2); > + active_l1 =3D true; > } > =20 > for(i =3D 0; i < l1_size; i++) { > @@ -1051,13 +1048,14 @@ fail: > } > =20 > ret =3D bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1= _size2); > - > - for (i =3D 0; i < l1_size; i++) { > - be64_to_cpus(&l1_table[i]); > + if (active_l1 && ret =3D=3D 0) { > + for (i =3D 0; i < l1_size; i++) { > + be64_to_cpus(&l1_table[i]); > + } > + memcpy(s->l1_table, l1_table, l1_size2); > } > } > - if (l1_allocated) > - g_free(l1_table); > + qemu_vfree(l1_table); > return ret; > } > =20 > --=20 > 1.9.3 >=20 >=20 --NDin8bjvE/0mNLFQ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUeE6nAAoJEJykq7OBq3PIqzEH/3yzZwWhu9+PmWUCW6kjIR4l MmyI/UGAs0ybHzRNcWMn7QkQi88+JuZRuLxV00EBHaKiSIihSElKqM6qAwWJylX6 4MMr0iOTsyQBZrtpwyRQlwRWgyi+tqlSWIJNrgdnure9KmSOsYkYAdp7uTFAf7Ul pagOBZL9WDaj2bLwi4KLHG1wAbkhGkFw5pM28KNjeNNkUk3skATyuZW/5cKiUmBF jSwDw3JTZndo7FE5MUYU0oaNrlksF6d49A60Xj6oQXpYWCvGc9Wm72+XyhGxXXYw ItlAJHhKK4i+feKi9pVEtp1GdCMU69xv+sFLfWLIDkTAjanYyu6AWMTICHhFGU4= =rT3i -----END PGP SIGNATURE----- --NDin8bjvE/0mNLFQ--