[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion |
Date: |
Thu, 12 Jan 2017 17:18:03 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
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 <address@hidden>
>> Cc: John Snow <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>> 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