[Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Ian Jackson]

Check that asynchronous (DMA) submission succeeds

If it does not, abort the command immediately rather than dropping
it on the floor.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 hw/ide.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index 1e60591..9b95c35 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -737,6 +737,13 @@ static inline void ide_abort_command(IDEState *s)
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
 }
+static inline void ide_dma_submit_check(IDEState *s,
+          BlockDriverCompletionFunc *dma_cb, BMDMAState *bm)
+{
+    if (bm->aiocb)
+	return;
+    dma_cb(bm, -1);
+}
 
 static inline void ide_set_irq(IDEState *s)
 {
@@ -933,6 +940,7 @@ static void ide_read_dma_cb(void *opaque, int ret)
 #endif
     bm->aiocb = bdrv_aio_read(s->bs, sector_num, s->io_buffer, n,
                               ide_read_dma_cb, bm);
+    ide_dma_submit_check(s, ide_read_dma_cb, bm);
 }
 
 static void ide_sector_read_dma(IDEState *s)
@@ -1035,6 +1043,7 @@ static void ide_write_dma_cb(void *opaque, int ret)
 #endif
     bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
                                ide_write_dma_cb, bm);
+    ide_dma_submit_check(s, ide_write_dma_cb, bm);
 }
 
 static void ide_sector_write_dma(IDEState *s)
-- 
1.4.4.4

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Anthony Liguori]

Ian Jackson wrote:
> Check that asynchronous (DMA) submission succeeds
>
> If it does not, abort the command immediately rather than dropping
> it on the floor.
>   

It's generally dangerous to call callbacks from the code that is issuing 
a bdrv_aio_ operation.  A malicious guest could potentially force the 
emulation into an infinite loop.

It would be better to use a bottom half to dispatch the callback.

Regards,

Anthony Liguori

> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  hw/ide.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/hw/ide.c b/hw/ide.c
> index 1e60591..9b95c35 100644
> --- a/hw/ide.c
> +++ b/hw/ide.c
> @@ -737,6 +737,13 @@ static inline void ide_abort_command(IDEState *s)
>      s->status = READY_STAT | ERR_STAT;
>      s->error = ABRT_ERR;
>  }
> +static inline void ide_dma_submit_check(IDEState *s,
> +          BlockDriverCompletionFunc *dma_cb, BMDMAState *bm)
> +{
> +    if (bm->aiocb)
> +	return;
> +    dma_cb(bm, -1);
> +}
>  
>  static inline void ide_set_irq(IDEState *s)
>  {
> @@ -933,6 +940,7 @@ static void ide_read_dma_cb(void *opaque, int ret)
>  #endif
>      bm->aiocb = bdrv_aio_read(s->bs, sector_num, s->io_buffer, n,
>                                ide_read_dma_cb, bm);
> +    ide_dma_submit_check(s, ide_read_dma_cb, bm);
>  }
>  
>  static void ide_sector_read_dma(IDEState *s)
> @@ -1035,6 +1043,7 @@ static void ide_write_dma_cb(void *opaque, int ret)
>  #endif
>      bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
>                                 ide_write_dma_cb, bm);
> +    ide_dma_submit_check(s, ide_write_dma_cb, bm);
>  }
>  
>  static void ide_sector_write_dma(IDEState *s)
>   

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds"):
> It's generally dangerous to call callbacks from the code that is issuing 
> a bdrv_aio_ operation.  A malicious guest could potentially force the 
> emulation into an infinite loop.

I'm not sure exactly what you mean but I'm sure that this is not a
problem in this case.  In my patch ide_dma_submit_check is called from
two places:
 * ide_read_dma_cb, with ide_read_dma_cb as the callback argument
 * ide_write_dma_cb, with ide_write_dma_cb as the callback argument

In both places the only situation where the callback is reentered
immediately is if the aio submission failed.  So in that case we
recursively enter the callback function, and we do so exactly once
since we're going to execute the error handling case (ret==-1).

Note that the call to ide_dma_submit_check is at the end of
ide_{read,write}_dma_cb precisely to avoid any kind of reentrancy
problem.

There is no possibility of any looping.  Effectively, in the error
case, we go back to the top of ide_{read,write}_dma_cb and execute the
existing error exit.

I'm not sure what you mean in terms of a malicious guest.  Surely it
is not surprising that a malicious guest can consume qemu CPU ?  Or do
you mean that it can consume qemu CPU indefinitely without needing to
do anything itself ?

But in any case if the arrangements in my patch are confusing or
likely to be broken by someone editing it in the future who doesn't
quite understand these matters, then perhaps we could address this
with a comment ?

Ian.

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Anthony Liguori]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds"):
>   
>> It's generally dangerous to call callbacks from the code that is issuing 
>> a bdrv_aio_ operation.  A malicious guest could potentially force the 
>> emulation into an infinite loop.
>>     
>
> I'm not sure exactly what you mean but I'm sure that this is not a
> problem in this case.  In my patch ide_dma_submit_check is called from
> two places:
>  * ide_read_dma_cb, with ide_read_dma_cb as the callback argument
>  * ide_write_dma_cb, with ide_write_dma_cb as the callback argument
>
> In both places the only situation where the callback is reentered
> immediately is if the aio submission failed.  So in that case we
> recursively enter the callback function, and we do so exactly once
> since we're going to execute the error handling case (ret==-1).
>
> Note that the call to ide_dma_submit_check is at the end of
> ide_{read,write}_dma_cb precisely to avoid any kind of reentrancy
> problem.
>   

I'll have to look more closely, but most of the code goes to great 
lengths to use bottom halves to avoid the possibility of infinite 
recursion.  The concern with recursion is not CPU consumption, it's that 
you'll eventually overrun the stack and potentially crash the QEMU process.

You may be right that in this case, recursion is impossible but it's 
probably better to use a bottom half just for the sake of consistency.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Jamie Lokier]

Anthony Liguori wrote:
> I'll have to look more closely, but most of the code goes to great 
> lengths to use bottom halves to avoid the possibility of infinite 
> recursion.  The concern with recursion is not CPU consumption, it's that 
> you'll eventually overrun the stack and potentially crash the QEMU process.

(Completely unrelated: "qemu -net nic -net user -net user" will
overrun the stack and crash the QEMU process as soon as the guest
makes an external TCP connection.)

-- Jamie

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Gleb Natapov]

On Thu, Aug 28, 2008 at 05:52:12PM +0100, Ian Jackson wrote:
> Check that asynchronous (DMA) submission succeeds
> 
> If it does not, abort the command immediately rather than dropping
> it on the floor.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  hw/ide.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ide.c b/hw/ide.c
> index 1e60591..9b95c35 100644
> --- a/hw/ide.c
> +++ b/hw/ide.c
> @@ -737,6 +737,13 @@ static inline void ide_abort_command(IDEState *s)
>      s->status = READY_STAT | ERR_STAT;
>      s->error = ABRT_ERR;
>  }
> +static inline void ide_dma_submit_check(IDEState *s,
> +          BlockDriverCompletionFunc *dma_cb, BMDMAState *bm)
> +{
> +    if (bm->aiocb)
> +	return;
> +    dma_cb(bm, -1);
> +}
>  
Currently neither ide_read_dma_cb() nor ide_write_dma_cb() checks ret
value. Was the error handling in a different patch that I've missed?


>  static inline void ide_set_irq(IDEState *s)
>  {
> @@ -933,6 +940,7 @@ static void ide_read_dma_cb(void *opaque, int ret)
>  #endif
>      bm->aiocb = bdrv_aio_read(s->bs, sector_num, s->io_buffer, n,
>                                ide_read_dma_cb, bm);
> +    ide_dma_submit_check(s, ide_read_dma_cb, bm);
>  }
>  
>  static void ide_sector_read_dma(IDEState *s)
> @@ -1035,6 +1043,7 @@ static void ide_write_dma_cb(void *opaque, int ret)
>  #endif
>      bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
>                                 ide_write_dma_cb, bm);
> +    ide_dma_submit_check(s, ide_write_dma_cb, bm);
>  }
>  
>  static void ide_sector_write_dma(IDEState *s)
> -- 
> 1.4.4.4
> 
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds

[Ian Jackson]

Gleb Natapov writes ("Re: [Qemu-devel] [PATCH] [ide] Check that asynchronous (DMA) submission succeeds"):
> Currently neither ide_read_dma_cb() nor ide_write_dma_cb() checks ret
> value. Was the error handling in a different patch that I've missed?

Yes, that error handling was in a different patch which seems to have
been dropped.  I will repost that dropped patch along with this IDE
dma change.

Sorry for the confusion; I was looking at my to-go-upstream branch
rather than the official one, although the patch I posted was against
the latter.

Ian.