* [Qemu-devel] [PATCH] ide: avoid unbounded recursion
@ 2017-01-12 10:51 Paolo Bonzini
2017-01-12 14:11 ` Stefan Hajnoczi
2017-01-12 15:54 ` John Snow
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2017-01-12 10:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Lieven, John Snow
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);
}
}
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
2017-01-12 10:51 [Qemu-devel] [PATCH] ide: avoid unbounded recursion Paolo Bonzini
@ 2017-01-12 14:11 ` Stefan Hajnoczi
2017-01-12 16:05 ` John Snow
2017-01-12 16:18 ` Paolo Bonzini
2017-01-12 15:54 ` John Snow
1 sibling, 2 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2017-01-12 14:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, John Snow, Peter Lieven
[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
2017-01-12 10:51 [Qemu-devel] [PATCH] ide: avoid unbounded recursion Paolo Bonzini
2017-01-12 14:11 ` Stefan Hajnoczi
@ 2017-01-12 15:54 ` John Snow
1 sibling, 0 replies; 6+ messages in thread
From: John Snow @ 2017-01-12 15:54 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Peter Lieven
On 01/12/2017 05:51 AM, 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);
> }
> }
>
>
Oh, is that all it took? Sorry for putting this off.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
2017-01-12 14:11 ` Stefan Hajnoczi
@ 2017-01-12 16:05 ` John Snow
2017-01-12 17:07 ` Paolo Bonzini
2017-01-12 16:18 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: John Snow @ 2017-01-12 16:05 UTC (permalink / raw)
To: Stefan Hajnoczi, Paolo Bonzini; +Cc: qemu-devel, Peter Lieven
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
2017-01-12 14:11 ` Stefan Hajnoczi
2017-01-12 16:05 ` John Snow
@ 2017-01-12 16:18 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2017-01-12 16:18 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Peter Lieven, John Snow, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion
2017-01-12 16:05 ` John Snow
@ 2017-01-12 17:07 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2017-01-12 17:07 UTC (permalink / raw)
To: John Snow, Stefan Hajnoczi; +Cc: Peter Lieven, qemu-devel
On 12/01/2017 17:05, John Snow wrote:
>
>
> 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?
Yes, but there's still migration.
> Also, I suppose drain doesn't clear out any scheduled bottom halves
> associated with the device, does it?
You could use bdrv_inc/dec_in_flight, but I'm not suggesting that seriously.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-12 17:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-12 10:51 [Qemu-devel] [PATCH] ide: avoid unbounded recursion Paolo Bonzini
2017-01-12 14:11 ` Stefan Hajnoczi
2017-01-12 16:05 ` John Snow
2017-01-12 17:07 ` Paolo Bonzini
2017-01-12 16:18 ` Paolo Bonzini
2017-01-12 15:54 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).