From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cRhsI-0004TR-46 for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:05:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cRhs8-0007Mm-SU for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:05:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57256) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cRhs8-0007MB-NV for qemu-devel@nongnu.org; Thu, 12 Jan 2017 11:05:04 -0500 References: <20170112105132.10394-1-pbonzini@redhat.com> <20170112141145.GA14042@stefanha-x1.localdomain> From: John Snow Message-ID: Date: Thu, 12 Jan 2017 11:05:03 -0500 MIME-Version: 1.0 In-Reply-To: <20170112141145.GA14042@stefanha-x1.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ide: avoid unbounded recursion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , Paolo Bonzini Cc: qemu-devel@nongnu.org, 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 >> Cc: John Snow >> Signed-off-by: Paolo Bonzini >> --- >> 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?