Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion

[John Snow <jsnow@redhat.com>]

On 01/12/2017 09:11 AM, Stefan Hajnoczi wrote:
> On Thu, Jan 12, 2017 at 11:51:32AM +0100, Paolo Bonzini wrote:
>> The end_transfer_func can call ide_transfer_start immediately, before
>> returning, and unbounded recursion can happen at least for
>> ide_atapi_cmd_reply_end.  Use a bottom half to defer the call and
>> limit stack usage.
>>
>> Cc: Peter Lieven <pl@kamp.de>
>> Cc: John Snow <jsnow@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/ide/core.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 43709e5..7b9831f 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -482,6 +482,13 @@ static void ide_clear_retry(IDEState *s)
>>      s->bus->retry_nsector = 0;
>>  }
>>  
>> +static void ide_start_transfer_bh_cb(void *opaque)
>> +{
>> +    IDEDMA *dma = opaque;
>> +
>> +    dma->ops->start_transfer(dma);
>> +}
>> +
>>  /* prepare data transfer and tell what to do after */
>>  void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>>                          EndTransferFunc *end_transfer_func)
>> @@ -494,7 +501,12 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>>          s->status |= DRQ_STAT;
>>      }
>>      if (s->bus->dma->ops->start_transfer) {
>> -        s->bus->dma->ops->start_transfer(s->bus->dma);
>> +        /* There can be unbounded recursion between ops->start_transfer
>> +         * and end_transfer_func, so defer to a bottom half.
>> +         */
>> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
>> +                                ide_start_transfer_bh_cb,
>> +                                s->bus->dma);
> 
> Are you sure this is safe?
> 
> I wonder if there are races with device reset, vmsave, or vcpu hw
> register accesses.
> 
> Stefan
> 

Oh, right, reset -- we're guarded against normal accesses because the
ATA device will be busy, but we're always allowed to reset.

Can we just amend the ide reset functionality to delete the BH if it
hasn't triggered yet?

Also, I suppose drain doesn't clear out any scheduled bottom halves
associated with the device, does it?