[Qemu-devel] [PATCH] qed: limit to image size in qed_find_cluster()

[Qemu-devel] [PATCH] qed: limit to image size in qed_find_cluster()

[Stefan Hajnoczi]

Callers of bdrv_is_allocated() may go beyond the end of the image.  For
general robustness we should limit to the end of the image so that
callers don't end up using out-of-range sector counts and receive -EIO.

This fix will allow the image streaming to terminate successfully.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/qed-cluster.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block/qed-cluster.c b/block/qed-cluster.c
index f64b2af..4632b32 100644
--- a/block/qed-cluster.c
+++ b/block/qed-cluster.c
@@ -142,6 +142,9 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
      */
     len = MIN(len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos);
 
+    /* Limit length to image size */
+    len = MIN(len, s->header.image_size - pos);
+
     l2_offset = s->l1_table->offsets[qed_l1_index(s, pos)];
     if (qed_offset_is_unalloc_cluster(l2_offset)) {
         cb(opaque, QED_CLUSTER_L1, 0, len);
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH] qed: limit to image size in qed_find_cluster()

[Mark Wu]

On 11/29/2011 12:18 AM, Stefan Hajnoczi wrote:
> Callers of bdrv_is_allocated() may go beyond the end of the image.  For
> general robustness we should limit to the end of the image so that
> callers don't end up using out-of-range sector counts and receive -EIO.
It seems the same problem exists in qcow/qcow2, why do we not add the 
limit in bdrv_is_allocated? Then it can cover all formats.

Actually, I can't figure out how the caller can go beyond the end of the 
image.  If the I/O request for sectors beyond image size comes from 
guest, it should be dropped in the guest block layer.  If the request 
comes from qemu, like block streaming, it should also honor the image 
size, right?

Mark.

Re: [Qemu-devel] [PATCH] qed: limit to image size in qed_find_cluster()

[Stefan Hajnoczi]

On Tue, Nov 29, 2011 at 5:01 AM, Mark Wu <wudxw@linux.vnet.ibm.com> wrote:
> On 11/29/2011 12:18 AM, Stefan Hajnoczi wrote:
>>
>> Callers of bdrv_is_allocated() may go beyond the end of the image.  For
>> general robustness we should limit to the end of the image so that
>> callers don't end up using out-of-range sector counts and receive -EIO.
>
> It seems the same problem exists in qcow/qcow2, why do we not add the limit
> in bdrv_is_allocated? Then it can cover all formats.
>
> Actually, I can't figure out how the caller can go beyond the end of the
> image.  If the I/O request for sectors beyond image size comes from guest,
> it should be dropped in the guest block layer.  If the request comes from
> qemu, like block streaming, it should also honor the image size, right?

Today's bdrv_is_allocated() implementation limits to the end of device
if BlockDriver.bdrv_is_allocated is NULL (there is a default
implementation).  This is a hint that callers may pass pnum values
beyond the end of the image.

Why is it useful to allow this?  Because it makes the caller's job
simpler.  They can pass in their buffer size directly instead of
checking for end of device.  The result from bdrv_is_allocated()
should limit to the end of device and the caller can safely use it.

I think we should preserve this behavior.  I like your idea of making
it generic and will do that in a new patch.

Stefan