From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:42242)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XdDgF-0006cQ-FQ
	for qemu-devel@nongnu.org; Sun, 12 Oct 2014 03:35:09 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XdDg9-00078h-6R
	for qemu-devel@nongnu.org; Sun, 12 Oct 2014 03:35:03 -0400
Received: from mx1.redhat.com ([209.132.183.28]:37174)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1XdDg8-00078O-WA
	for qemu-devel@nongnu.org; Sun, 12 Oct 2014 03:34:57 -0400
Date: Sun, 12 Oct 2014 09:34:32 +0200
From: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20141012073432.GA3739@noname.redhat.com>
References: <201410111514227991260@sangfor.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <201410111514227991260@sangfor.com>
Subject: Re: [Qemu-devel] [PATCH] qcow2: fix double-free of
 Qcow2DiscardRegion in qcow2_process_discards
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Zhang Haoyu <zhanghy@sangfor.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>, qemu-devel <qemu-devel@nongnu.org>, kvm <kvm@vger.kernel.org>

Am 11.10.2014 um 09:14 hat Zhang Haoyu geschrieben:
> In qcow2_update_snapshot_refcount -> qcow2_process_discards() -> bdrv_discard()
> may free the Qcow2DiscardRegion which is referenced by "next" pointer in
> qcow2_process_discards() now, in next iteration, d = next, so g_free(d)
> will double-free this Qcow2DiscardRegion.
> 
> qcow2_snapshot_delete
> |- qcow2_update_snapshot_refcount
> |-- qcow2_process_discards
> |--- bdrv_discard
> |---- aio_poll
> |----- aio_dispatch
> |------ bdrv_co_io_em_complete
> |------- qemu_coroutine_enter(co->coroutine, NULL); <=== coroutine entry is bdrv_co_do_rw
> |--- g_free(d) <== free first Qcow2DiscardRegion is okay
> |--- d = next;  <== this set is done in QTAILQ_FOREACH_SAFE() macro.
> |--- g_free(d);  <== double-free will happen if during previous iteration, bdrv_discard had free this object.

Do you have a reproducer for this or did code review lead you to this?

At the moment I can't see how bdrv_discard(bs->file) could ever free a
Qcow2DiscardRegion of bs, as it's working on a completely different
BlockDriverState (which usually won't even be a qcow2 one).

> bdrv_co_do_rw
> |- bdrv_co_do_writev
> |-- bdrv_co_do_pwritev
> |--- bdrv_aligned_pwritev
> |---- qcow2_co_writev
> |----- qcow2_alloc_cluster_link_l2
> |------ qcow2_free_any_clusters
> |------- qcow2_free_clusters
> |-------- update_refcount
> |--------- qcow2_process_discards
> |---------- g_free(d)  <== In next iteration, this Qcow2DiscardRegion will be double-free.

This shouldn't happen in a nested call either, as s->lock can't be taken
recursively.

Kevin