Re: [Qemu-devel] [RFC 04/10] ide: Correct handling of malformed/short PRDTs

[Paolo Bonzini]

Il 13/09/2014 06:34, John Snow ha scritto:
> This impacts both BMDMA and AHCI HBA interfaces for IDE.
> Currently, we confuse the difference between a PRD having
> "0 bytes" and a PRD having "0 complete sectors."
> 
> This leads to, in the BMDMA case, leaked memory for short PRDTs,
> and infinite loops in the AHCI case.
> 
> the "prepare_buf" callback is reworked to return 0 if it could
> not allocate a full sector's worth of buffer space, instead of
> returning non-zero if it allocated any number of bytes.
> 
> ide_dma_cb adds a call to commit_buf in order to delete
> the short PRDT that it will not attempt to use to finish
> the DMA operation.
> 
> This patch corrects both occurrences and adds an assertion to
> prevent future regression. This assertion is tested in the
> existing ide-test, and is covered in a forthcoming AHCI test.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  dma-helpers.c | 3 +++
>  hw/ide/ahci.c | 2 +-
>  hw/ide/core.c | 1 +
>  hw/ide/pci.c  | 5 +++--
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index ba965a3..3f9766d 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, 
> uint8_t c, dma_addr_t len)
>  void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
>                        AddressSpace *as)
>  {
> +    /* If this is true, you're leaking memory. */
> +    assert(qsg->sg == NULL);
> +
>      qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>      qsg->nsg = 0;
>      qsg->nalloc = alloc_hint;
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 8e6a352..42a77c4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1132,7 +1132,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int 
> is_write)
>      s->io_buffer_size = s->sg.size;
>  
>      DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
> -    return s->io_buffer_size != 0;
> +    return s->io_buffer_size / 512 != 0;
>  }
>  
>  /**
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b2980e9..1685f6d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -726,6 +726,7 @@ void ide_dma_cb(void *opaque, int ret)
>          /* The PRDs were too short. Reset the Active bit, but don't raise an
>           * interrupt. */
>          s->status = READY_STAT | SEEK_STAT;
> +        dma_buf_commit(s, false);
>          goto eot;
>      }
>  
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 2397f35..3f643c2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
>          if (bm->cur_prd_len == 0) {
>              /* end of table (with a fail safe of one page) */
>              if (bm->cur_prd_last ||
> -                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
> -                return s->io_buffer_size != 0;
> +                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
> +                return (s->io_buffer_size / 512) != 0;
> +            }
>              pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
>              bm->cur_addr += 8;
>              prd.addr = le32_to_cpu(prd.addr);
> 

Reviewed-by: Paolo Bonzini <address@hidden>

The changes I suggested in patch 2 shouldn't be a hurdle here.

Paolo