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

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/01/2017 15:11, 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.

Register accesses are okay.  For device reset and vmsave I had actually
thought about that, but I had thought about it wrong.  

Device reset and vmsave drain the BlockBackend, but there's nothing to 
drain for most PIO commands.  Device reset could just cancel a
(non-oneshot) bottom half, but I'm not sure of how to handle vmsave.
Maybe do only part of start_transfer in the bottom half so that the
destination has some condition to check (s->data_ptr < s->data_end?)
and can reschedule the bottom half on the destination side depending
on that condition?

Paolo