[RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

libFuzzer triggered the following assertion:

  cat << EOF | qemu-system-i386 -M pc-q35-5.0 \
    -nographic -monitor none -serial none \
    -qtest stdio -trace ide\*
  outl 0xcf8 0x8000fa24
  outl 0xcfc 0xe106c000
  outl 0xcf8 0x8000fa04
  outw 0xcfc 0x7
  outl 0xcf8 0x8000fb20
  write 0x0 0x3 0x2780e7
  write 0xe106c22c 0xd 0x1130c218021130c218021130c2
  write 0xe106c218 0x15 0x110010110010110010110010110010110010110010
  EOF
  ide_exec_cmd IDE exec cmd: bus 0x56170a77a2b8; state 0x56170a77a340; cmd 0xe7
  ide_reset IDEstate 0x56170a77a340
  Aborted (core dumped)

  (gdb) bt
  #1  0x00007ffff4f93895 in abort () at /lib64/libc.so.6
  #2  0x0000555555dc6c00 in bdrv_aio_cancel (acb=0x555556765550) at block/io.c:2745
  #3  0x0000555555dac202 in blk_aio_cancel (acb=0x555556765550) at block/block-backend.c:1546
  #4  0x0000555555b1bd74 in ide_reset (s=0x555557213340) at hw/ide/core.c:1318
  #5  0x0000555555b1e3a1 in ide_bus_reset (bus=0x5555572132b8) at hw/ide/core.c:2422
  #6  0x0000555555b2aa27 in ahci_reset_port (s=0x55555720eb50, port=2) at hw/ide/ahci.c:650
  #7  0x0000555555b29fd7 in ahci_port_write (s=0x55555720eb50, port=2, offset=44, val=16) at hw/ide/ahci.c:360
  #8  0x0000555555b2a564 in ahci_mem_write (opaque=0x55555720eb50, addr=556, val=16, size=1) at hw/ide/ahci.c:513
  #9  0x000055555598415b in memory_region_write_accessor (mr=0x55555720eb80, addr=556, value=0x7fffffffb838, size=1, shift=0, mask=255, attrs=...) at softmmu/memory.c:483

Looking at bdrv_aio_cancel:

2728 /* async I/Os */
2729
2730 void bdrv_aio_cancel(BlockAIOCB *acb)
2731 {
2732     qemu_aio_ref(acb);
2733     bdrv_aio_cancel_async(acb);
2734     while (acb->refcnt > 1) {
2735         if (acb->aiocb_info->get_aio_context) {
2736             aio_poll(acb->aiocb_info->get_aio_context(acb), true);
2737         } else if (acb->bs) {
2738             /* qemu_aio_ref and qemu_aio_unref are not thread-safe, so
2739              * assert that we're not using an I/O thread.  Thread-safe
2740              * code should use bdrv_aio_cancel_async exclusively.
2741              */
2742             assert(bdrv_get_aio_context(acb->bs) == qemu_get_aio_context());
2743             aio_poll(bdrv_get_aio_context(acb->bs), true);
2744         } else {
2745             abort();     <===============
2746         }
2747     }
2748     qemu_aio_unref(acb);
2749 }

Our context is already referenced but we don't have a getter,
neither a block driver state. Maybe because we are called from
a vCPU context? Avoid this case by calling the pending callback
directly. In this case this is WIN_FLUSH_CACHE. I'm not sure for
the other READ/WRITE commands.

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: bef0fd5958 ("ide: convert ide_sector_read() to asynchronous I/O")
BugLink: https://bugs.launchpad.net/qemu/+bug/1878255
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because I don't understand AIO operations well.
After RFC Cc: qemu-stable@nongnu.org

ide_is_pio_out() verifies a PIO OUT checking:

  s->end_transfer_func == ide_dummy_transfer_stop

An alternative might be:

    if (s->pio_aiocb && s->end_transfer_func == ide_dummy_transfer_stop) {
        /* If there is a pending AIO callback, invoke it now. */
        blk_aio_cancel_async(s->pio_aiocb);
        s->pio_aiocb = NULL;
    }

Or if we want to limit to WIN_FLUSH_CACHE:

    if (s->pio_aiocb && s->bus->error_status & IDE_RETRY_FLUSH) {
        /* If there is a pending AIO callback, invoke it now. */
        blk_aio_cancel_async(s->pio_aiocb);
        s->pio_aiocb = NULL;
    }

Cc: Stefan Hajnoczi <stefanha@redhat.com>

Last minute chat:
19:01 <stefanha> f4bug: use bdrv_aio_cancel_async() if possible because it won't block the current thread.
19:02 <stefanha> f4bug: For example, in device emulation code where the guest has requested to cancel an I/O request it's often possible to use the async version.
19:02 <stefanha> f4bug: But in synchronous code like device reset it may be necessary to use the synchronous (blocking) bdrv_aio_cancel() API instead. :(
19:14 <stefanha> f4bug: The way to decide is: will the current function return to the event loop and is there someone who will handle the request completion callback when cancel finishes?
19:14 <stefanha> f4bug: If the next line of code requires the request to finished then async cancel cannot be used.
19:15 <stefanha> f4bug: On the other hand, if the function can return and it's okay for the request to cancel at some future time then you can use async.

So I'll revisit this patch :)
---
 hw/ide/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..e3a9ce7d25 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1315,7 +1315,8 @@ static void ide_reset(IDEState *s)
     trace_ide_reset(s);
 
     if (s->pio_aiocb) {
-        blk_aio_cancel(s->pio_aiocb);
+        /* If there is a pending AIO callback, invoke it now. */
+        blk_aio_cancel_async(s->pio_aiocb);
         s->pio_aiocb = NULL;
     }
 
-- 
2.21.3