[Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range

[Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range

[Anton Nefedov]

Started from the separate series discussion (trim statistics) , see
http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01059.html

There is no range check for IDE trim requests now.
Such request will likely be rejected by the block layer and count as
failed and not an invalid/aborted operation.

Anton Nefedov (3):
  ide: pass IDEState to trim AIO callback
  ide: move ide_sect_range_ok() up
  ide: abort TRIM operation for invalid range

 hw/ide/core.c | 53 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] ide: pass IDEState to trim AIO callback

[Anton Nefedov]

It will be needed to handle invalid requests

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 hw/ide/core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 471d0c9..02a6b2c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -383,7 +383,7 @@ static void ide_set_signature(IDEState *s)
 
 typedef struct TrimAIOCB {
     BlockAIOCB common;
-    BlockBackend *blk;
+    IDEState *s;
     QEMUBH *bh;
     int ret;
     QEMUIOVector *qiov;
@@ -426,6 +426,8 @@ static void ide_trim_bh_cb(void *opaque)
 static void ide_issue_trim_cb(void *opaque, int ret)
 {
     TrimAIOCB *iocb = opaque;
+    IDEState *s = iocb->s;
+
     if (ret >= 0) {
         while (iocb->j < iocb->qiov->niov) {
             int j = iocb->j;
@@ -443,7 +445,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
                 }
 
                 /* Got an entry! Submit and exit.  */
-                iocb->aiocb = blk_aio_pdiscard(iocb->blk,
+                iocb->aiocb = blk_aio_pdiscard(s->blk,
                                                sector << BDRV_SECTOR_BITS,
                                                count << BDRV_SECTOR_BITS,
                                                ide_issue_trim_cb, opaque);
@@ -467,11 +469,11 @@ BlockAIOCB *ide_issue_trim(
         int64_t offset, QEMUIOVector *qiov,
         BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
 {
-    BlockBackend *blk = opaque;
+    IDEState *s = opaque;
     TrimAIOCB *iocb;
 
-    iocb = blk_aio_get(&trim_aiocb_info, blk, cb, cb_opaque);
-    iocb->blk = blk;
+    iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
+    iocb->s = s;
     iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
     iocb->ret = 0;
     iocb->qiov = qiov;
@@ -901,7 +903,7 @@ static void ide_dma_cb(void *opaque, int ret)
     case IDE_DMA_TRIM:
         s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
                                         &s->sg, offset, BDRV_SECTOR_SIZE,
-                                        ide_issue_trim, s->blk, ide_dma_cb, s,
+                                        ide_issue_trim, s, ide_dma_cb, s,
                                         DMA_DIRECTION_TO_DEVICE);
         break;
     default:
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] ide: move ide_sect_range_ok() up

[Anton Nefedov]

to use it without a forward declaration in the commit to follow

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 hw/ide/core.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 02a6b2c..23c71fa 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -381,6 +381,18 @@ static void ide_set_signature(IDEState *s)
     }
 }
 
+static bool ide_sect_range_ok(IDEState *s,
+                              uint64_t sector, uint64_t nb_sectors)
+{
+    uint64_t total_sectors;
+
+    blk_get_geometry(s->blk, &total_sectors);
+    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
+        return false;
+    }
+    return true;
+}
+
 typedef struct TrimAIOCB {
     BlockAIOCB common;
     IDEState *s;
@@ -604,18 +616,6 @@ 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;
-
-    blk_get_geometry(s->blk, &total_sectors);
-    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
-        return false;
-    }
-    return true;
-}
-
 static void ide_buffered_readv_cb(void *opaque, int ret)
 {
     IDEBufferedRequest *req = opaque;
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] ide: abort TRIM operation for invalid range

[Anton Nefedov]

ATA8-ACS3, 7.9 DATA SET MANAGEMENT - 06h, DMA

    7.9.5 Error Outputs
    If the Trim bit is set to one and:
      a) the device detects an invalid LBA Range Entry; or
      b) count is greater than IDENTIFY DEVICE data word 105
         (see 7.16.7.55),
    then the device shall return command aborted.
    A device may trim one or more LBA Range Entries before it returns
    command aborted. See table 209.

This check is not in the common ide_dma_cb() as the range for TRIM
is harder to reach: it is not in LBA/count registers and the buffer has
to be parsed first.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 hw/ide/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 23c71fa..3d1494f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -401,6 +401,7 @@ typedef struct TrimAIOCB {
     QEMUIOVector *qiov;
     BlockAIOCB *aiocb;
     int i, j;
+    bool is_invalid;
 } TrimAIOCB;
 
 static void trim_aio_cancel(BlockAIOCB *acb)
@@ -428,8 +429,11 @@ static void ide_trim_bh_cb(void *opaque)
 {
     TrimAIOCB *iocb = opaque;
 
-    iocb->common.cb(iocb->common.opaque, iocb->ret);
-
+    if (iocb->is_invalid) {
+        ide_dma_error(iocb->s);
+    } else {
+        iocb->common.cb(iocb->common.opaque, iocb->ret);
+    }
     qemu_bh_delete(iocb->bh);
     iocb->bh = NULL;
     qemu_aio_unref(iocb);
@@ -456,6 +460,11 @@ static void ide_issue_trim_cb(void *opaque, int ret)
                     continue;
                 }
 
+                if (!ide_sect_range_ok(s, sector, count)) {
+                    iocb->is_invalid = true;
+                    goto done;
+                }
+
                 /* Got an entry! Submit and exit.  */
                 iocb->aiocb = blk_aio_pdiscard(s->blk,
                                                sector << BDRV_SECTOR_BITS,
@@ -471,6 +480,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
         iocb->ret = ret;
     }
 
+done:
     iocb->aiocb = NULL;
     if (iocb->bh) {
         qemu_bh_schedule(iocb->bh);
@@ -491,6 +501,7 @@ BlockAIOCB *ide_issue_trim(
     iocb->qiov = qiov;
     iocb->i = -1;
     iocb->j = 0;
+    iocb->is_invalid = false;
     ide_issue_trim_cb(iocb, 0);
     return &iocb->common;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] ide: abort TRIM operation for invalid range

[John Snow]

On 12/08/2017 07:10 AM, Anton Nefedov wrote:
> ATA8-ACS3, 7.9 DATA SET MANAGEMENT - 06h, DMA
> 
>     7.9.5 Error Outputs
>     If the Trim bit is set to one and:
>       a) the device detects an invalid LBA Range Entry; or
>       b) count is greater than IDENTIFY DEVICE data word 105
>          (see 7.16.7.55),
>     then the device shall return command aborted.
>     A device may trim one or more LBA Range Entries before it returns
>     command aborted. See table 209.
> 
> This check is not in the common ide_dma_cb() as the range for TRIM
> is harder to reach: it is not in LBA/count registers and the buffer has
> to be parsed first.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  hw/ide/core.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 23c71fa..3d1494f 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -401,6 +401,7 @@ typedef struct TrimAIOCB {
>      QEMUIOVector *qiov;
>      BlockAIOCB *aiocb;
>      int i, j;
> +    bool is_invalid;
>  } TrimAIOCB;
>  
>  static void trim_aio_cancel(BlockAIOCB *acb)
> @@ -428,8 +429,11 @@ static void ide_trim_bh_cb(void *opaque)
>  {
>      TrimAIOCB *iocb = opaque;
>  
> -    iocb->common.cb(iocb->common.opaque, iocb->ret);
> -
> +    if (iocb->is_invalid) {
> +        ide_dma_error(iocb->s);
> +    } else {
> +        iocb->common.cb(iocb->common.opaque, iocb->ret);
> +    }
>      qemu_bh_delete(iocb->bh);
>      iocb->bh = NULL;
>      qemu_aio_unref(iocb);
> @@ -456,6 +460,11 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>                      continue;
>                  }
>  
> +                if (!ide_sect_range_ok(s, sector, count)) {
> +                    iocb->is_invalid = true;
> +                    goto done;
> +                }
> +
>                  /* Got an entry! Submit and exit.  */
>                  iocb->aiocb = blk_aio_pdiscard(s->blk,
>                                                 sector << BDRV_SECTOR_BITS,
> @@ -471,6 +480,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>          iocb->ret = ret;
>      }
>  
> +done:
>      iocb->aiocb = NULL;
>      if (iocb->bh) {
>          qemu_bh_schedule(iocb->bh);
> @@ -491,6 +501,7 @@ BlockAIOCB *ide_issue_trim(
>      iocb->qiov = qiov;
>      iocb->i = -1;
>      iocb->j = 0;
> +    iocb->is_invalid = false;
>      ide_issue_trim_cb(iocb, 0);
>      return &iocb->common;
>  }>

Looks about right, just remember that this flow won't call
block_acct_invalid because you're bypassing the return to ide_dma_cb. I
assume you'll get to that in your next series.

For now, this should properly reject bogus TRIM commands. When you send
your next series, may I ask for a simple test case if possible?

1-3:
Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] ide: abort TRIM operation for invalid range

[Anton Nefedov]

On 8/12/2017 10:51 PM, John Snow wrote:
> 
> Looks about right, just remember that this flow won't call
> block_acct_invalid because you're bypassing the return to ide_dma_cb. I
> assume you'll get to that in your next series.
> 

Yes; I meant to keep the trim accounting in ide_issue_trim_cb()

> For now, this should properly reject bogus TRIM commands. When you send
> your next series, may I ask for a simple test case if possible?
> 

Sure, I'll look into it

> 1-3:
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

Re: [Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range

[John Snow]

On 12/08/2017 07:10 AM, Anton Nefedov wrote:
> Started from the separate series discussion (trim statistics) , see
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01059.html
> 
> There is no range check for IDE trim requests now.
> Such request will likely be rejected by the block layer and count as
> failed and not an invalid/aborted operation.
> 
> Anton Nefedov (3):
>   ide: pass IDEState to trim AIO callback
>   ide: move ide_sect_range_ok() up
>   ide: abort TRIM operation for invalid range
> 
>  hw/ide/core.c | 53 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

(PR won't be sent until after the 2.12 tree opens.)

Re: [Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range

[John Snow]

On 12/08/2017 07:10 AM, Anton Nefedov wrote:
> Started from the separate series discussion (trim statistics) , see
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01059.html
> 
> There is no range check for IDE trim requests now.
> Such request will likely be rejected by the block layer and count as
> failed and not an invalid/aborted operation.
> 
> Anton Nefedov (3):
>   ide: pass IDEState to trim AIO callback
>   ide: move ide_sect_range_ok() up
>   ide: abort TRIM operation for invalid range
> 
>  hw/ide/core.c | 53 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 

I forgot about this series due to the 2.11 freeze and winter break. It
appears to still apply, so I'll send it along.