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
Patchew URL: https://patchew.org/QEMU/20200717171938.1249-1-f4bug@amsat.org/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/target/arm/translate-sve.o
CC aarch64-softmmu/trace/generated-helpers.o
LINK aarch64-softmmu/qemu-system-aarch64
collect2: error: ld returned 1 exit status
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-aarch64] Error 1
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
make: *** [x86_64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 708, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3bb1a90ba07d44f8a68428ba8cc1aac8', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-s7o9muvt/src/docker-src.2020-07-17-13.24.31.3740:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3bb1a90ba07d44f8a68428ba8cc1aac8
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-s7o9muvt/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m44.977s
user 0m8.984s
The full log is available at
http://patchew.org/logs/20200717171938.1249-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Fri, Jul 17, 2020 at 07:19:38PM +0200, Philippe Mathieu-Daudé wrote:
> 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);
This is a place where an async call is not allowed. The completion
function must be called right away (synchronously) before we can
continue resetting the device.
I sent a patch that allows bdrv_aio_cancel() to find the AioContext so
it can call aio_poll().
Stefan
On 7/20/20 6:02 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 17, 2020 at 07:19:38PM +0200, Philippe Mathieu-Daudé wrote:
>> 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);
>
> This is a place where an async call is not allowed. The completion
> function must be called right away (synchronously) before we can
> continue resetting the device.
>
> I sent a patch that allows bdrv_aio_cancel() to find the AioContext so
> it can call aio_poll().
>
> Stefan
>
OK, dropping Phil's patch here.
--js
© 2016 - 2025 Red Hat, Inc.