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 <address@hidden>
> ---
>  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