qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] qcow2: Check bs->drv in copy_sectors()


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 1/3] qcow2: Check bs->drv in copy_sectors()
Date: Tue, 11 Mar 2014 15:04:31 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

Il 11/03/2014 11:16, Kevin Wolf ha scritto:
Am 11.03.2014 um 00:16 hat Laszlo Ersek geschrieben:
On 03/10/14 23:44, Max Reitz wrote:
Before dereferencing bs->drv for a call to its member bdrv_co_readv(),
copy_sectors() should check whether that pointer is indeed valid, since
it may have been set to NULL by e.g. a concurrent write triggering the
corruption prevention mechanism.

Signed-off-by: Max Reitz <address@hidden>
---
To be precise, this still is a race condition. If bs->drv is set to NULL
after the check and before the call to bdrv_co_readv(), QEMU will
obviously still crash. However, in order to circumvent this behavior, we
would probably have to re-lock s->lock, check bs->drv, take the function
pointer to bdrv_co_readv() and then unlock s->lock before the function
is called. I found this rather ugly and therefore this still has a very
small chance of running into a race condition.
Therefore, I'm asking for your opinion on this, whether we can really
take this chance or should rather "do it right". In fact, if I were a
reviewer, I'd probably reject this patch and request the solution with
the function pointer (if there is no better solution), but I was afraid
to send such an ugly patch.

No, the code is fine. Remember that qcow2 is not threaded, we're talking
about coroutines here. There is no way for the code to yield between
your check and the protected place.

 block/qcow2-cluster.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 36c1bed..9499df9 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -380,6 +380,10 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,

     BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);

+    if (!bs->drv) {
+        return -ENOMEDIUM;
+    }
+
     /* Call .bdrv_co_readv() directly instead of using the public block-layer
      * interface.  This avoids double I/O throttling and request tracking,
      * which can lead to deadlock when block layer copy-on-read is enabled.


I can't answer your question nor review this patch -- instead, I have a
question of my own: when you say "set to NULL by [...] the corruption
prevention mechanism", do you mean qcow2_pre_write_overlap_check():

        bs->drv = NULL; /* make BDS unusable */

Yes, this is the place.

If so: I thought that it was quite a bold move, but also that we'd find
the SIGSEGVs sooner or later... :)

In fact, if you use the block layer API, most functions check for
bs->drv and return -ENOMEDIUM if it is NULL. The problem here is that we
directly dereference the pointer without going through block.c (there's
a good reason for this, see the comment, but it still makes it somewhat
special).

But why not call qcow2_co_readv directly?

Paolo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]