@samitouri / QOSamiQemu / commits / 5bdc12fed5

hw/ide/ahci: drain the ports on teardown

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 <jsnow@redhat.com> Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> Signed-off-by: Denis V. Lunev <den@openvz.org>

Denis V. Lunev committed Aug 18, 2026 at 09:55 UTC 5bdc12fed592a1673f64ca1e8243046632f83a26
1 file changed +49 -23
hw/ide/ahci.c
+49 -23
@@ -619,12 +619,37 @@ static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
619 s->lcyl, s->hcyl, sig);
620 }
621
622 +static void ahci_cancel_ncq_requests(AHCIDevice *ad)
623 +{
624 + int i;
625 +
626 + for (i = 0; i < AHCI_MAX_CMDS; i++) {
627 + NCQTransferState *ncq_tfs = &ad->ncq_tfs[i];
628 + ncq_tfs->halt = false;
629 + if (!ncq_tfs->used) {
630 + continue;
631 + }
632 +
633 + if (ncq_tfs->aiocb) {
634 + blk_aio_cancel(ncq_tfs->aiocb);
635 + ncq_tfs->aiocb = NULL;
636 + }
637 +
638 + /* Maybe we just finished the request thanks to blk_aio_cancel() */
639 + if (!ncq_tfs->used) {
640 + continue;
641 + }
642 +
643 + qemu_sglist_destroy(&ncq_tfs->sglist);
644 + ncq_tfs->used = 0;
645 + }
646 +}
647 +
648 static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind)
649 {
650 AHCIDevice *d = &s->dev[port];
651 AHCIPortRegs *pr = &d->port_regs;
652 IDEState *ide_state = &d->port.ifs[0];
627 - int i;
653
654 trace_ahci_reset_port(s, port);
655
@@ -645,27 +670,7 @@ static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind)
670 return;
671 }
672
648 - /* reset ncq queue */
649 - for (i = 0; i < AHCI_MAX_CMDS; i++) {
650 - NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[i];
651 - ncq_tfs->halt = false;
652 - if (!ncq_tfs->used) {
653 - continue;
654 - }
655 -
656 - if (ncq_tfs->aiocb) {
657 - blk_aio_cancel(ncq_tfs->aiocb);
658 - ncq_tfs->aiocb = NULL;
659 - }
660 -
661 - /* Maybe we just finished the request thanks to blk_aio_cancel() */
662 - if (!ncq_tfs->used) {
663 - continue;
664 - }
665 -
666 - qemu_sglist_destroy(&ncq_tfs->sglist);
667 - ncq_tfs->used = 0;
668 - }
673 + ahci_cancel_ncq_requests(d);
674
675 s->dev[port].port_state = STATE_RUN;
676 if (ide_state->drive_kind == IDE_CD) {
@@ -1659,8 +1664,29 @@ void ahci_uninit(AHCIState *s)
1664 for (i = 0; i < s->ports; i++) {
1665 AHCIDevice *ad = &s->dev[i];
1666
1667 + /*
1668 + * Unplug does not go through a reset, so this is the only chance to
1669 + * detach the requests and the bottom half that would otherwise walk
1670 + * s->dev after it is freed below.
1671 + */
1672 + ahci_cancel_ncq_requests(ad);
1673 + if (ad->check_bh) {
1674 + qemu_bh_delete(ad->check_bh);
1675 + ad->check_bh = NULL;
1676 + }
1677 +
1678 for (j = 0; j < 2; j++) {
1663 - ide_exit(&ad->port.ifs[j]);
1679 + IDEState *ide_state = &ad->port.ifs[j];
1680 +
1681 + /*
1682 + * Everything the port still owns points into the allocation this
1683 + * function frees, io_buffer included, so nothing may be left in
1684 + * flight once ide_exit() has run.
1685 + */
1686 + if (ide_state->blk) {
1687 + blk_drain(ide_state->blk);
1688 + }
1689 + ide_exit(ide_state);
1690 }
1691 object_unparent(OBJECT(&ad->port));
1692 }