|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH 1/3] qcow2: Check bs->drv in copy_sectors() |
Date: | Tue, 11 Mar 2014 19:19:08 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
On 11.03.2014 15:04, Paolo Bonzini wrote:
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, sinceit 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 NULLafter the check and before the call to bdrv_co_readv(), QEMU willobviously still crash. However, in order to circumvent this behavior, we would probably have to re-lock s->lock, check bs->drv, take the functionpointer to bdrv_co_readv() and then unlock s->lock before the functionis called. I found this rather ugly and therefore this still has a verysmall 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 withthe function pointer (if there is no better solution), but I was afraidto 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?
qcow2_co_readv() would probably actually perform the operation, as far as I can see, since it does not check whether the medium is corrupt (this is only checked when opening the block device). This is the reason why bs->drv is set to NULL, so noone is able to read from the block device anymore, even without checking for corruption (additionally, on version 2 images, there is not even a way to indicate corruption other than setting bs->drv to NULL). Normally, every read operation would pass through bdrv_co_readv(), which will (have to) check whether bs->drv is valid or NULL - since this code decides to skip that and call the driver function directly, we therefore have to check bs->drv anyway, since it is the only way of telling us whether the image may actually be accessed right here. We could change the bs->drv->bdrv_co_readv() call to qcow2_co_readv(), but then people will probably ask why there is a bs->drv check right before.
Max
[Prev in Thread] | Current Thread | [Next in Thread] |