From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: qemu-devel@nongnu.org
Cc: "Stefan Hajnoczi" <stefanha@redhat.com>,
"Alexander Bulekov" <alxndr@bu.edu>,
"John Snow" <jsnow@redhat.com>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
qemu-block@nongnu.org
Subject: [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive
Date: Fri, 17 Jul 2020 19:19:38 +0200 [thread overview]
Message-ID: <20200717171938.1249-1-f4bug@amsat.org> (raw)
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
next reply other threads:[~2020-07-17 17:20 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-17 17:19 Philippe Mathieu-Daudé [this message]
2020-07-17 17:27 ` [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive no-reply
2020-07-20 10:02 ` Stefan Hajnoczi
2020-07-21 16:37 ` John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200717171938.1249-1-f4bug@amsat.org \
--to=f4bug@amsat.org \
--cc=alxndr@bu.edu \
--cc=jsnow@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.