* [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 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 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
* 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 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
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).