[Qemu-devel] [PATCH v2 2.1 0/4] Suppress error action on r/w beyond end

[Qemu-devel] [PATCH v2 2.1 0/4] Suppress error action on r/w beyond end

[Markus Armbruster]

When a device model's I/O operation fails, we execute the error
action.  This lets layers above QEMU implement thin provisioning, or
attempt to correct errors before they reach the guest.  But when the
I/O operation fails because its invalid, reporting the error to the
guest is the only sensible action.

SCSI does that, but virtio-blk and IDE don't.  Fix them.  No other
device model supports error actions.

v2:
* PATCH 2: Commit message spelling fix [Fam]
* PATCH 4: New, covering IDE

Markus Armbruster (4):
  virtio-blk: Factor common checks out of virtio_blk_handle_read/write()
  virtio-blk: Bypass error action and I/O accounting on invalid r/w
  virtio-blk: Treat read/write beyond end as invalid
  ide: Treat read/write beyond end as invalid

 hw/block/virtio-blk.c | 45 +++++++++++++++++++++++++++++----------------
 hw/ide/core.c         | 28 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 16 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 1/4] virtio-blk: Factor common checks out of virtio_blk_handle_read/write()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index aec3146..d946fa9 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -288,6 +288,18 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     bdrv_aio_flush(req->dev->bs, virtio_blk_flush_complete, req);
 }
 
+static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
+                                     uint64_t sector, size_t size)
+{
+    if (sector & dev->sector_mask) {
+        return false;
+    }
+    if (size % dev->conf->logical_block_size) {
+        return false;
+    }
+    return true;
+}
+
 static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     BlockRequest *blkreq;
@@ -299,11 +311,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 
     trace_virtio_blk_handle_write(req, sector, req->qiov.size / 512);
 
-    if (sector & req->dev->sector_mask) {
-        virtio_blk_rw_complete(req, -EIO);
-        return;
-    }
-    if (req->qiov.size % req->dev->conf->logical_block_size) {
+    if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
         virtio_blk_rw_complete(req, -EIO);
         return;
     }
@@ -333,11 +341,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 
     trace_virtio_blk_handle_read(req, sector, req->qiov.size / 512);
 
-    if (sector & req->dev->sector_mask) {
-        virtio_blk_rw_complete(req, -EIO);
-        return;
-    }
-    if (req->qiov.size % req->dev->conf->logical_block_size) {
+    if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
         virtio_blk_rw_complete(req, -EIO);
         return;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 2/4] virtio-blk: Bypass error action and I/O accounting on invalid r/w

[Markus Armbruster]

When a device model's I/O operation fails, we execute the error
action.  This lets layers above QEMU implement thin provisioning, or
attempt to correct errors before they reach the guest.  But when the
I/O operation fails because it's invalid, reporting the error to the
guest is the only sensible action.

If the guest's read or write asks for an invalid sector range, fail
the request right away, without considering the error action.  No
change with error action BDRV_ACTION_REPORT.

Furthermore, bypass I/O accounting, because we want to track only I/O
that actually reaches the block layer.

The next commit will extend "invalid sector range" to cover attempts
to read/write beyond the end of the medium.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index d946fa9..c8952c0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -307,15 +307,16 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 
     sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &req->out.sector);
 
-    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
-
     trace_virtio_blk_handle_write(req, sector, req->qiov.size / 512);
 
     if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
-        virtio_blk_rw_complete(req, -EIO);
+        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
+        g_free(req);
         return;
     }
 
+    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
+
     if (mrb->num_writes == 32) {
         virtio_submit_multiwrite(req->dev->bs, mrb);
     }
@@ -337,14 +338,15 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 
     sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &req->out.sector);
 
-    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
-
     trace_virtio_blk_handle_read(req, sector, req->qiov.size / 512);
 
     if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
-        virtio_blk_rw_complete(req, -EIO);
+        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
+        g_free(req);
         return;
     }
+
+    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
     bdrv_aio_readv(req->dev->bs, sector, &req->qiov,
                    req->qiov.size / BDRV_SECTOR_SIZE,
                    virtio_blk_rw_complete, req);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2.1 2/4] virtio-blk: Bypass error action and I/O accounting on invalid r/w

[Kevin Wolf]

Am 04.07.2014 um 15:31 hat Markus Armbruster geschrieben:
> When a device model's I/O operation fails, we execute the error
> action.  This lets layers above QEMU implement thin provisioning, or
> attempt to correct errors before they reach the guest.  But when the
> I/O operation fails because it's invalid, reporting the error to the
> guest is the only sensible action.
> 
> If the guest's read or write asks for an invalid sector range, fail
> the request right away, without considering the error action.  No
> change with error action BDRV_ACTION_REPORT.
> 
> Furthermore, bypass I/O accounting, because we want to track only I/O
> that actually reaches the block layer.
> 
> The next commit will extend "invalid sector range" to cover attempts
> to read/write beyond the end of the medium.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/virtio-blk.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index d946fa9..c8952c0 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -307,15 +307,16 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>  
>      sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &req->out.sector);
>  
> -    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
> -
>      trace_virtio_blk_handle_write(req, sector, req->qiov.size / 512);
>  
>      if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
> -        virtio_blk_rw_complete(req, -EIO);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +        g_free(req);

This should probably be virtio_blk_free_request() now, which was only
recently introduced.

>          return;
>      }
>  
> +    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
> +
>      if (mrb->num_writes == 32) {
>          virtio_submit_multiwrite(req->dev->bs, mrb);
>      }
> @@ -337,14 +338,15 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
>  
>      sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &req->out.sector);
>  
> -    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
> -
>      trace_virtio_blk_handle_read(req, sector, req->qiov.size / 512);
>  
>      if (!virtio_blk_sect_range_ok(req->dev, sector, req->qiov.size)) {
> -        virtio_blk_rw_complete(req, -EIO);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +        g_free(req);

Same here.

>          return;
>      }
> +
> +    bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
>      bdrv_aio_readv(req->dev->bs, sector, &req->qiov,
>                     req->qiov.size / BDRV_SECTOR_SIZE,
>                     virtio_blk_rw_complete, req);

Kevin

[Qemu-devel] [PATCH v2 2.1 3/4] virtio-blk: Treat read/write beyond end as invalid

[Markus Armbruster]

The block layer fails such reads and writes just fine.  However, they
then get treated like valid operations that fail: the error action
gets executed.  Unwanted; reporting the error to the guest is the only
sensible action.

Reject them before passing them to the block layer.  This bypasses the
error action and I/O accounting.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c8952c0..5ec8469 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -291,12 +291,19 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
                                      uint64_t sector, size_t size)
 {
+    uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
+    uint64_t total_sectors;
+
     if (sector & dev->sector_mask) {
         return false;
     }
     if (size % dev->conf->logical_block_size) {
         return false;
     }
+    bdrv_get_geometry(dev->bs, &total_sectors);
+    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
+        return false;
+    }
     return true;
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 4/4] ide: Treat read/write beyond end as invalid

[Markus Armbruster]

The block layer fails such reads and writes just fine.  However, they
then get treated like valid operations that fail: the error action
gets executed.  Unwanted; reporting the error to the guest is the only
sensible action.

Reject them before passing them to the block layer.  This bypasses the
error action and, for PIO but not DMA, I/O accounting.  Tolerable,
because I/O accounting is an inconsistent mess anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3a38f1e..63a500d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -499,6 +499,18 @@ static void ide_rw_error(IDEState *s) {
     ide_set_irq(s->bus);
 }
 
+static bool ide_sect_range_ok(IDEState *s,
+                              uint64_t sector, uint64_t nb_sectors)
+{
+    uint64_t total_sectors;
+
+    bdrv_get_geometry(s->bs, &total_sectors);
+    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
+        return false;
+    }
+    return true;
+}
+
 static void ide_sector_read_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
@@ -554,6 +566,11 @@ void ide_sector_read(IDEState *s)
     printf("sector=%" PRId64 "\n", sector_num);
 #endif
 
+    if (!ide_sect_range_ok(s, sector_num, n)) {
+        ide_rw_error(s);
+        return;
+    }
+
     s->iov.iov_base = s->io_buffer;
     s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&s->qiov, &s->iov, 1);
@@ -671,6 +688,12 @@ void ide_dma_cb(void *opaque, int ret)
            sector_num, n, s->dma_cmd);
 #endif
 
+    if (!ide_sect_range_ok(s, sector_num, n)) {
+        dma_buf_commit(s);
+        ide_dma_error(s);
+        goto eot;
+    }
+
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
         s->bus->dma->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
@@ -790,6 +813,11 @@ void ide_sector_write(IDEState *s)
         n = s->req_nb_sectors;
     }
 
+    if (!ide_sect_range_ok(s, sector_num, n)) {
+        ide_rw_error(s);
+        return;
+    }
+
     s->iov.iov_base = s->io_buffer;
     s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&s->qiov, &s->iov, 1);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2.1 4/4] ide: Treat read/write beyond end as invalid

[Kevin Wolf]

Am 04.07.2014 um 15:32 hat Markus Armbruster geschrieben:
> The block layer fails such reads and writes just fine.  However, they
> then get treated like valid operations that fail: the error action
> gets executed.  Unwanted; reporting the error to the guest is the only
> sensible action.
> 
> Reject them before passing them to the block layer.  This bypasses the
> error action and, for PIO but not DMA, I/O accounting.  Tolerable,
> because I/O accounting is an inconsistent mess anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3a38f1e..63a500d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -499,6 +499,18 @@ static void ide_rw_error(IDEState *s) {
>      ide_set_irq(s->bus);
>  }
>  
> +static bool ide_sect_range_ok(IDEState *s,
> +                              uint64_t sector, uint64_t nb_sectors)
> +{
> +    uint64_t total_sectors;
> +
> +    bdrv_get_geometry(s->bs, &total_sectors);
> +    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void ide_sector_read_cb(void *opaque, int ret)
>  {
>      IDEState *s = opaque;
> @@ -554,6 +566,11 @@ void ide_sector_read(IDEState *s)
>      printf("sector=%" PRId64 "\n", sector_num);
>  #endif
>  
> +    if (!ide_sect_range_ok(s, sector_num, n)) {
> +        ide_rw_error(s);
> +        return;
> +    }
> +
>      s->iov.iov_base = s->io_buffer;
>      s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
>      qemu_iovec_init_external(&s->qiov, &s->iov, 1);
> @@ -671,6 +688,12 @@ void ide_dma_cb(void *opaque, int ret)
>             sector_num, n, s->dma_cmd);
>  #endif
>  
> +    if (!ide_sect_range_ok(s, sector_num, n)) {
> +        dma_buf_commit(s);
> +        ide_dma_error(s);
> +        goto eot;

Are you sure that this should be 'goto eot' rather than just 'return'?
When jumping to eot, we do the I/O accounting (which we said we don't
care about) and call ide_set_inactive() for a second time. The condition
for setting BM_STATUS_DMAING is never met when coming from here.

I am worried about ide_set_inactive() doing double request cleanup.

Kevin

Re: [Qemu-devel] [PATCH v2 2.1 4/4] ide: Treat read/write beyond end as invalid

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.07.2014 um 15:32 hat Markus Armbruster geschrieben:
>> The block layer fails such reads and writes just fine.  However, they
>> then get treated like valid operations that fail: the error action
>> gets executed.  Unwanted; reporting the error to the guest is the only
>> sensible action.
>> 
>> Reject them before passing them to the block layer.  This bypasses the
>> error action and, for PIO but not DMA, I/O accounting.  Tolerable,
>> because I/O accounting is an inconsistent mess anyway.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/ide/core.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 3a38f1e..63a500d 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -499,6 +499,18 @@ static void ide_rw_error(IDEState *s) {
>>      ide_set_irq(s->bus);
>>  }
>>  
>> +static bool ide_sect_range_ok(IDEState *s,
>> +                              uint64_t sector, uint64_t nb_sectors)
>> +{
>> +    uint64_t total_sectors;
>> +
>> +    bdrv_get_geometry(s->bs, &total_sectors);
>> +    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>>  static void ide_sector_read_cb(void *opaque, int ret)
>>  {
>>      IDEState *s = opaque;
>> @@ -554,6 +566,11 @@ void ide_sector_read(IDEState *s)
>>      printf("sector=%" PRId64 "\n", sector_num);
>>  #endif
>>  
>> +    if (!ide_sect_range_ok(s, sector_num, n)) {
>> +        ide_rw_error(s);
>> +        return;
>> +    }
>> +
>>      s->iov.iov_base = s->io_buffer;
>>      s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
>>      qemu_iovec_init_external(&s->qiov, &s->iov, 1);
>> @@ -671,6 +688,12 @@ void ide_dma_cb(void *opaque, int ret)
>>             sector_num, n, s->dma_cmd);
>>  #endif
>>  
>> +    if (!ide_sect_range_ok(s, sector_num, n)) {
>> +        dma_buf_commit(s);
>> +        ide_dma_error(s);
>> +        goto eot;
>
> Are you sure that this should be 'goto eot' rather than just 'return'?
> When jumping to eot, we do the I/O accounting (which we said we don't
> care about) and call ide_set_inactive() for a second time. The condition
> for setting BM_STATUS_DMAING is never met when coming from here.
>
> I am worried about ide_set_inactive() doing double request cleanup.

You're right; I missed the fact that ide_dma_error() calls
ide_set_inactive() already.

Immediate return also skips the other things happening after eot, but
that's okay, because:

* skipping the bdrv_acct_done() merely changes I/O accounting to be
  busted somewhat differently, and

* stay_active is certainly false, so we don't actually skip anything
  there.

Respin sent.