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

[Markus Armbruster]

Kevin Wolf <address@hidden> 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 <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.

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.