From: Denis V. Lunev <[email protected]> ahci_uninit() frees s->dev without touching the requests still in flight. The only blk_aio_cancel() for them lives in ahci_reset_port(), which the unplug path does not run, and the ide-hd child's own drain is deferred through call_rcu so it happens after the free. A guest that powers the root port slot off through SLTCTL, or writes the ACPI ejection register, while a read is outstanding therefore leaves the completion to run against freed memory.
A plain device_del is not affected: the pciehp attention-button flow resets the secondary bus first, which cancels through the reset path. Surprise removal is what skips it. Cancelling the NCQ requests alone is not enough. IDEDMA and IDEBus are embedded in AHCIDevice, so a plain DMA read reaches the freed array through dma_blk_cb() and a PIO read through ide_buffered_readv_cb(), neither of which the NCQ bookkeeping covers. ide_exit() drains nothing and frees io_buffer, which an outstanding request may still target. Move the NCQ cancel loop into a helper, run it from ahci_uninit() too, and drain each port before ide_exit() so no class of request can outlive the allocation. Delete check_bh there as well; qemu_bh_new_guarded() in check_cmd() has no counterpart on this path either. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4069 Cc: John Snow <[email protected]> Cc: Philippe Mathieu-Daudé <[email protected]> Signed-off-by: Denis V. Lunev <[email protected]> --- hw/ide/ahci.c | 72 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 23 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 2b2ef873e0..6b04762c4a 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -619,12 +619,37 @@ static void ahci_set_signature(AHCIDevice *ad, uint32_t sig) s->lcyl, s->hcyl, sig); } +static void ahci_cancel_ncq_requests(AHCIDevice *ad) +{ + int i; + + for (i = 0; i < AHCI_MAX_CMDS; i++) { + NCQTransferState *ncq_tfs = &ad->ncq_tfs[i]; + ncq_tfs->halt = false; + if (!ncq_tfs->used) { + continue; + } + + if (ncq_tfs->aiocb) { + blk_aio_cancel(ncq_tfs->aiocb); + ncq_tfs->aiocb = NULL; + } + + /* Maybe we just finished the request thanks to blk_aio_cancel() */ + if (!ncq_tfs->used) { + continue; + } + + qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_tfs->used = 0; + } +} + static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind) { AHCIDevice *d = &s->dev[port]; AHCIPortRegs *pr = &d->port_regs; IDEState *ide_state = &d->port.ifs[0]; - int i; trace_ahci_reset_port(s, port); @@ -645,27 +670,7 @@ static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind) return; } - /* reset ncq queue */ - for (i = 0; i < AHCI_MAX_CMDS; i++) { - NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[i]; - ncq_tfs->halt = false; - if (!ncq_tfs->used) { - continue; - } - - if (ncq_tfs->aiocb) { - blk_aio_cancel(ncq_tfs->aiocb); - ncq_tfs->aiocb = NULL; - } - - /* Maybe we just finished the request thanks to blk_aio_cancel() */ - if (!ncq_tfs->used) { - continue; - } - - qemu_sglist_destroy(&ncq_tfs->sglist); - ncq_tfs->used = 0; - } + ahci_cancel_ncq_requests(d); s->dev[port].port_state = STATE_RUN; if (ide_state->drive_kind == IDE_CD) { @@ -1659,8 +1664,29 @@ void ahci_uninit(AHCIState *s) for (i = 0; i < s->ports; i++) { AHCIDevice *ad = &s->dev[i]; + /* + * Unplug does not go through a reset, so this is the only chance to + * detach the requests and the bottom half that would otherwise walk + * s->dev after it is freed below. + */ + ahci_cancel_ncq_requests(ad); + if (ad->check_bh) { + qemu_bh_delete(ad->check_bh); + ad->check_bh = NULL; + } + for (j = 0; j < 2; j++) { - ide_exit(&ad->port.ifs[j]); + IDEState *ide_state = &ad->port.ifs[j]; + + /* + * Everything the port still owns points into the allocation this + * function frees, io_buffer included, so nothing may be left in + * flight once ide_exit() has run. + */ + if (ide_state->blk) { + blk_drain(ide_state->blk); + } + ide_exit(ide_state); } object_unparent(OBJECT(&ad->port)); } -- 2.53.0
