[Qemu-devel] [PATCH 0/2] ide/hw/core: fix bug# 1777315, crash on short PRDs

[Qemu-devel] [PATCH 0/2] ide/hw/core: fix bug# 1777315, crash on short PRDs

[Amol Surati]

The fix utilizes the existing policy QEMU has about short PRDs, and
considers the transfers that cause the crash as generated through short
PRDS.

It
- continues to allow QEMU to support multiple calls to
  prepare_buf/ide_dma_cb,
- so, continues to keep QEMU free from needing the entire sglist in one
  go;
- avoids the crash;
- but, treats the affected transfers as short, instead of allowing them
  to continue.


Amol Surati (1):
  ide/hw/core: fix crash on processing a partial-sector-size DMA xfer

John Snow (1):
  tests/ide-test: test case for crash when processing short PRDs

 hw/ide/core.c    |  5 ++++-
 tests/ide-test.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer

[Amol Surati]

Fixes: https://bugs.launchpad.net/qemu/+bug/1777315

QEMU's short PRD policy applies to a DMA transfer of size < 512 bytes.
But it fails to consider transfers which are >= 512 bytes, but are
not a multiple of 512 bytes.

Such transfers are not subject to the short PRD policy. They end up
violating the assumptions about the granularity of the IO sizes,
upon which depend the verification of the completion of the previous
transfer, and the advancement of the offset in preparation of the next.

Those violations result in the crash.

By forcing each transfer to be a multiple of sector size, such
transfers are subjected to the policy, and therefore culled before they
cause the crash.

Signed-off-by: Amol Surati <suratiamol@gmail.com>
---
 hw/ide/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 2c62efc536..14d135224b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -836,6 +836,7 @@ static void ide_dma_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
     int n;
+    int32_t size_prepared;
     int64_t sector_num;
     uint64_t offset;
     bool stay_active = false;
@@ -886,7 +887,9 @@ static void ide_dma_cb(void *opaque, int ret)
     n = s->nsector;
     s->io_buffer_index = 0;
     s->io_buffer_size = n * 512;
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
+    size_prepared = s->bus->dma->ops->prepare_buf(s->bus->dma,
+                                                  s->io_buffer_size);
+    if (size_prepared <= 0 || size_prepared % 512) {
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
         s->status = READY_STAT | SEEK_STAT;
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] tests/ide-test: test case for crash when processing short PRDs

[Amol Surati]

From: John Snow <jsnow@redhat.com>

Related Bug: https://bugs.launchpad.net/qemu/+bug/1777315

Signed-off-by: Amol Surati <suratiamol@gmail.com>
---
 tests/ide-test.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index f39431b1a9..382c29a174 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -473,6 +473,32 @@ static void test_bmdma_one_sector_short_prdt(void)
     free_pci_device(dev);
 }
 
+static void test_bmdma_partial_sector_short_prdt(void)
+{
+    QPCIDevice *dev;
+    QPCIBar bmdma_bar, ide_bar;
+    uint8_t status;
+
+    /* Read 2 sectors but only give 1 sector in PRDT */
+    PrdtEntry prdt[] = {
+        {
+            .addr = 0,
+            .size = cpu_to_le32(0x200),
+        },
+        {
+            .addr = 512,
+            .size = cpu_to_le32(0x44 | PRDT_EOT),
+        }
+    };
+
+    dev = get_pci_device(&bmdma_bar, &ide_bar);
+    status = send_dma_request(CMD_READ_DMA, 0, 2,
+                              prdt, ARRAY_SIZE(prdt), NULL);
+    g_assert_cmphex(status, ==, 0);
+    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+    free_pci_device(dev);
+}
+
 static void test_bmdma_long_prdt(void)
 {
     QPCIDevice *dev;
@@ -1037,6 +1063,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
     qtest_add_func("/ide/bmdma/one_sector_short_prdt",
                    test_bmdma_one_sector_short_prdt);
+    qtest_add_func("/ide/bmdma/partial_sector_short_prdt",
+                   test_bmdma_partial_sector_short_prdt);
     qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
     qtest_add_func("/ide/bmdma/no_busmaster", test_bmdma_no_busmaster);
     qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer

[John Snow]

On 06/20/2018 12:29 AM, Amol Surati wrote:
> Fixes: https://bugs.launchpad.net/qemu/+bug/1777315
> 
> QEMU's short PRD policy applies to a DMA transfer of size < 512 bytes.
> But it fails to consider transfers which are >= 512 bytes, but are
> not a multiple of 512 bytes.
> 
> Such transfers are not subject to the short PRD policy. They end up
> violating the assumptions about the granularity of the IO sizes,
> upon which depend the verification of the completion of the previous
> transfer, and the advancement of the offset in preparation of the next.
> 
> Those violations result in the crash.
> 
> By forcing each transfer to be a multiple of sector size, such
> transfers are subjected to the policy, and therefore culled before they
> cause the crash.
> 

So now even if the PRDT we get is greater than a sector is not an even
multiple of 512, we reject it as too short.

That doesn't seem correct to me.

> Signed-off-by: Amol Surati <suratiamol@gmail.com>
> ---
>  hw/ide/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 2c62efc536..14d135224b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -836,6 +836,7 @@ static void ide_dma_cb(void *opaque, int ret)
>  {
>      IDEState *s = opaque;
>      int n;
> +    int32_t size_prepared;
>      int64_t sector_num;
>      uint64_t offset;
>      bool stay_active = false;
> @@ -886,7 +887,9 @@ static void ide_dma_cb(void *opaque, int ret)
>      n = s->nsector;
>      s->io_buffer_index = 0;
>      s->io_buffer_size = n * 512;
> -    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
> +    size_prepared = s->bus->dma->ops->prepare_buf(s->bus->dma,
> +                                                  s->io_buffer_size);
> +    if (size_prepared <= 0 || size_prepared % 512) {
>          /* The PRDs were too short. Reset the Active bit, but don't raise an
>           * interrupt. */
>          s->status = READY_STAT | SEEK_STAT;
> 

-- 
—js

Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer

[Amol Surati]

On Mon, Jun 25, 2018 at 05:10:23PM -0400, John Snow wrote:
> 
> 
> On 06/20/2018 12:29 AM, Amol Surati wrote:
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1777315
> > 
> > QEMU's short PRD policy applies to a DMA transfer of size < 512 bytes.
> > But it fails to consider transfers which are >= 512 bytes, but are
> > not a multiple of 512 bytes.
> > 
> > Such transfers are not subject to the short PRD policy. They end up
> > violating the assumptions about the granularity of the IO sizes,
> > upon which depend the verification of the completion of the previous
> > transfer, and the advancement of the offset in preparation of the next.
> > 
> > Those violations result in the crash.
> > 
> > By forcing each transfer to be a multiple of sector size, such
> > transfers are subjected to the policy, and therefore culled before they
> > cause the crash.
> > 
> 
> So now even if the PRDT we get is greater than a sector is not an even
> multiple of 512, we reject it as too short.

Yes. 

When PRDT is greater than a sector but non an even multiple of 512,
we used to crash. Now we do not.

The reason for this patch is to maintain backward compatibility with the
current split-completion design that QEMU adopts, while still avoiding the
crash. The cover letter has the reasons for this patch.


> 
> That doesn't seem correct to me.

https://github.com/asurati/1777315/blob/master/v2.diff has a different
patch, which aligns with what Kevin had suggested. It allows the
IO which you described above, but then it completes the command
as soon as it detects a partial transfer which is larger than 512.

The patch where s->sg.size is used as the offset to start the next
IO would require the dma IOs to be misaligned and of non-512 sizes.
Will that be okay? 

Else, one needs to change each implementation
of prepare_buf to ensure that it always ALIGNS_UP to the sector
size any partial read/write/trim, in anticipation of the next PRDT
which can cover the difference.


> 
> > Signed-off-by: Amol Surati <suratiamol@gmail.com>
> > ---
> >  hw/ide/core.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 2c62efc536..14d135224b 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -836,6 +836,7 @@ static void ide_dma_cb(void *opaque, int ret)
> >  {
> >      IDEState *s = opaque;
> >      int n;
> > +    int32_t size_prepared;
> >      int64_t sector_num;
> >      uint64_t offset;
> >      bool stay_active = false;
> > @@ -886,7 +887,9 @@ static void ide_dma_cb(void *opaque, int ret)
> >      n = s->nsector;
> >      s->io_buffer_index = 0;
> >      s->io_buffer_size = n * 512;
> > -    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
> > +    size_prepared = s->bus->dma->ops->prepare_buf(s->bus->dma,
> > +                                                  s->io_buffer_size);
> > +    if (size_prepared <= 0 || size_prepared % 512) {
> >          /* The PRDs were too short. Reset the Active bit, but don't raise an
> >           * interrupt. */
> >          s->status = READY_STAT | SEEK_STAT;
> > 
> 
> -- 
> —js