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 <alx...@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é <f4...@amsat.org>
---
RFC because I don't understand AIO operations well.
After RFC Cc: qemu-sta...@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 <stefa...@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
