[PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain

Denis V. Lunev via qemu development posted 9 patches 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260619112158.304782-1-den@openvz.org
Maintainers: John Snow <jsnow@redhat.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/ide/ahci.c             |   3 +
hw/ide/atapi.c            | 180 +++++++++++++++-----------------
tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
tests/qtest/libqos/ahci.c |   8 ++
tests/qtest/libqos/ahci.h |   2 +
6 files changed, 433 insertions(+), 172 deletions(-)
[PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Denis V. Lunev via qemu development 1 month, 1 week ago
An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
concurrent port operation, and it breaks in two distinct ways. Both are
guest-triggerable and reproduce on current master.

Neither was found by inspection: both are real issues reported against
the qemu-kvm 10.0 downstream -- the crash from production, and the
deadlock from the downstream live-migration validation suite -- on
CD-ROMs backed by high-latency network storage, where a backend read
outlives the guest's SATA command timeout.

1. Command engine restart -> NULL-deref crash (patches 1-2)

   A guest can stop and restart a port's command engine
   (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
   the command list and clears AHCIDevice.cur_cmd, but nothing tears
   down the outstanding read; its completion then dereferences the
   cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
   not the COMRESET path -- an engine restart does not run ide_reset(),
   so the drive's transfer state is preserved and the read still runs.

   Patch 1 cancels the outstanding I/O when the command list is
   unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
   RESET already does from the same MMIO context. Patch 2 is a qtest
   covering the PIO and DMA reply paths.

2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)

   A multi-sector PIO read whose byte-count limit spans more than one
   sector must fetch the later sectors of a DRQ burst from inside the
   completion of the first, asynchronous read. cd_read_sector_sync()
   did that with a synchronous blk_pread(), which queues behind any
   drain in progress. When a guest reset reaches
   virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
   read is in flight, the nested read is queued while the outer
   completion still holds blk->in_flight, so the drain never finishes:
   the main loop wedges in the drain with the BQL held and every
   QMP/monitor operation blocks behind it.

   Patch 8 reads the whole elementary transfer in a single asynchronous
   request, so no read is ever issued mid-burst; cd_read_sector_sync()
   is removed and the DMA path is unchanged. Patch 9 reproduces the
   hang via x-blockdev-set-iothread, which takes the same
   bdrv_drain_all_begin() path a guest reset does.

Patches 3-7 are the test groundwork the deadlock fix needs: they fold
the IDE CD-ROM read tests into one parametrized helper and add the
previously missing coverage -- multi-sector DMA, and raw 2352-byte
READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
(including its in-place 2352 unpack) is exercised on both consumers.
The test patches pass with or without patch 8, keeping the series
bisectable.

Patches 1-2 stand alone as the engine-restart crash fix; they are
included here because the deadlock work shares and extends the same
ATAPI read path and its test scaffolding.

checkpatch is clean on all nine patches; the new and existing IDE and
AHCI qtests pass.

Denis V. Lunev (9):
  hw/ide/ahci: cancel in-flight buffered reads on command engine restart
  tests/qtest/ahci: test ATAPI read completing after engine restart
  tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
  tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
  tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
  tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
  tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
  hw/ide/atapi: read the whole elementary transfer asynchronously
  tests/qtest/ahci: regression test for ATAPI read vs. drain

 hw/ide/ahci.c             |   3 +
 hw/ide/atapi.c            | 180 +++++++++++++++-----------------
 tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
 tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
 tests/qtest/libqos/ahci.c |   8 ++
 tests/qtest/libqos/ahci.h |   2 +
 6 files changed, 433 insertions(+), 172 deletions(-)

-- 
2.53.0
Re: [PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Denis V. Lunev 6 days, 3 hours ago
On 6/19/26 13:21, Denis V. Lunev wrote:
> An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
> the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
> concurrent port operation, and it breaks in two distinct ways. Both are
> guest-triggerable and reproduce on current master.
>
> Neither was found by inspection: both are real issues reported against
> the qemu-kvm 10.0 downstream -- the crash from production, and the
> deadlock from the downstream live-migration validation suite -- on
> CD-ROMs backed by high-latency network storage, where a backend read
> outlives the guest's SATA command timeout.
>
> 1. Command engine restart -> NULL-deref crash (patches 1-2)
>
>    A guest can stop and restart a port's command engine
>    (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
>    the command list and clears AHCIDevice.cur_cmd, but nothing tears
>    down the outstanding read; its completion then dereferences the
>    cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
>    not the COMRESET path -- an engine restart does not run ide_reset(),
>    so the drive's transfer state is preserved and the read still runs.
>
>    Patch 1 cancels the outstanding I/O when the command list is
>    unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
>    RESET already does from the same MMIO context. Patch 2 is a qtest
>    covering the PIO and DMA reply paths.
>
> 2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
>
>    A multi-sector PIO read whose byte-count limit spans more than one
>    sector must fetch the later sectors of a DRQ burst from inside the
>    completion of the first, asynchronous read. cd_read_sector_sync()
>    did that with a synchronous blk_pread(), which queues behind any
>    drain in progress. When a guest reset reaches
>    virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
>    read is in flight, the nested read is queued while the outer
>    completion still holds blk->in_flight, so the drain never finishes:
>    the main loop wedges in the drain with the BQL held and every
>    QMP/monitor operation blocks behind it.
>
>    Patch 8 reads the whole elementary transfer in a single asynchronous
>    request, so no read is ever issued mid-burst; cd_read_sector_sync()
>    is removed and the DMA path is unchanged. Patch 9 reproduces the
>    hang via x-blockdev-set-iothread, which takes the same
>    bdrv_drain_all_begin() path a guest reset does.
>
> Patches 3-7 are the test groundwork the deadlock fix needs: they fold
> the IDE CD-ROM read tests into one parametrized helper and add the
> previously missing coverage -- multi-sector DMA, and raw 2352-byte
> READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
> (including its in-place 2352 unpack) is exercised on both consumers.
> The test patches pass with or without patch 8, keeping the series
> bisectable.
>
> Patches 1-2 stand alone as the engine-restart crash fix; they are
> included here because the deadlock work shares and extends the same
> ATAPI read path and its test scaffolding.
>
> checkpatch is clean on all nine patches; the new and existing IDE and
> AHCI qtests pass.
>
> Denis V. Lunev (9):
>   hw/ide/ahci: cancel in-flight buffered reads on command engine restart
>   tests/qtest/ahci: test ATAPI read completing after engine restart
>   tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
>   tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
>   tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
>   tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
>   tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
>   hw/ide/atapi: read the whole elementary transfer asynchronously
>   tests/qtest/ahci: regression test for ATAPI read vs. drain
>
>  hw/ide/ahci.c             |   3 +
>  hw/ide/atapi.c            | 180 +++++++++++++++-----------------
>  tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
>  tests/qtest/libqos/ahci.c |   8 ++
>  tests/qtest/libqos/ahci.h |   2 +
>  6 files changed, 433 insertions(+), 172 deletions(-)
>
Hi!

if nobody is able to take a look, will it be fine if I'll
send pull request from my name for this?

Den
Re: [PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Philippe Mathieu-Daudé 5 days, 14 hours ago
On 20/7/26 09:58, Denis V. Lunev wrote:
> On 6/19/26 13:21, Denis V. Lunev wrote:
>> An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
>> the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
>> concurrent port operation, and it breaks in two distinct ways. Both are
>> guest-triggerable and reproduce on current master.
>>
>> Neither was found by inspection: both are real issues reported against
>> the qemu-kvm 10.0 downstream -- the crash from production, and the
>> deadlock from the downstream live-migration validation suite -- on
>> CD-ROMs backed by high-latency network storage, where a backend read
>> outlives the guest's SATA command timeout.
>>
>> 1. Command engine restart -> NULL-deref crash (patches 1-2)
>>
>>     A guest can stop and restart a port's command engine
>>     (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
>>     the command list and clears AHCIDevice.cur_cmd, but nothing tears
>>     down the outstanding read; its completion then dereferences the
>>     cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
>>     not the COMRESET path -- an engine restart does not run ide_reset(),
>>     so the drive's transfer state is preserved and the read still runs.
>>
>>     Patch 1 cancels the outstanding I/O when the command list is
>>     unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
>>     RESET already does from the same MMIO context. Patch 2 is a qtest
>>     covering the PIO and DMA reply paths.
>>
>> 2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
>>
>>     A multi-sector PIO read whose byte-count limit spans more than one
>>     sector must fetch the later sectors of a DRQ burst from inside the
>>     completion of the first, asynchronous read. cd_read_sector_sync()
>>     did that with a synchronous blk_pread(), which queues behind any
>>     drain in progress. When a guest reset reaches
>>     virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
>>     read is in flight, the nested read is queued while the outer
>>     completion still holds blk->in_flight, so the drain never finishes:
>>     the main loop wedges in the drain with the BQL held and every
>>     QMP/monitor operation blocks behind it.
>>
>>     Patch 8 reads the whole elementary transfer in a single asynchronous
>>     request, so no read is ever issued mid-burst; cd_read_sector_sync()
>>     is removed and the DMA path is unchanged. Patch 9 reproduces the
>>     hang via x-blockdev-set-iothread, which takes the same
>>     bdrv_drain_all_begin() path a guest reset does.
>>
>> Patches 3-7 are the test groundwork the deadlock fix needs: they fold
>> the IDE CD-ROM read tests into one parametrized helper and add the
>> previously missing coverage -- multi-sector DMA, and raw 2352-byte
>> READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
>> (including its in-place 2352 unpack) is exercised on both consumers.
>> The test patches pass with or without patch 8, keeping the series
>> bisectable.
>>
>> Patches 1-2 stand alone as the engine-restart crash fix; they are
>> included here because the deadlock work shares and extends the same
>> ATAPI read path and its test scaffolding.
>>
>> checkpatch is clean on all nine patches; the new and existing IDE and
>> AHCI qtests pass.
>>
>> Denis V. Lunev (9):
>>    hw/ide/ahci: cancel in-flight buffered reads on command engine restart
>>    tests/qtest/ahci: test ATAPI read completing after engine restart
>>    tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
>>    tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
>>    tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
>>    tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
>>    tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
>>    hw/ide/atapi: read the whole elementary transfer asynchronously
>>    tests/qtest/ahci: regression test for ATAPI read vs. drain
>>
>>   hw/ide/ahci.c             |   3 +
>>   hw/ide/atapi.c            | 180 +++++++++++++++-----------------
>>   tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
>>   tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
>>   tests/qtest/libqos/ahci.c |   8 ++
>>   tests/qtest/libqos/ahci.h |   2 +
>>   6 files changed, 433 insertions(+), 172 deletions(-)
>>
> Hi!
> 
> if nobody is able to take a look, will it be fine if I'll
> send pull request from my name for this?

qemu-block@ is Cc'ed so no objection on my side but first send a
MAINTAINERS patch stepping up for IDE, so you can include as first
patch of the pullreq?
Re: [PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Denis V. Lunev 2 weeks, 2 days ago
On 6/19/26 13:21, Denis V. Lunev wrote:
> An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
> the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
> concurrent port operation, and it breaks in two distinct ways. Both are
> guest-triggerable and reproduce on current master.
>
> Neither was found by inspection: both are real issues reported against
> the qemu-kvm 10.0 downstream -- the crash from production, and the
> deadlock from the downstream live-migration validation suite -- on
> CD-ROMs backed by high-latency network storage, where a backend read
> outlives the guest's SATA command timeout.
>
> 1. Command engine restart -> NULL-deref crash (patches 1-2)
>
>    A guest can stop and restart a port's command engine
>    (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
>    the command list and clears AHCIDevice.cur_cmd, but nothing tears
>    down the outstanding read; its completion then dereferences the
>    cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
>    not the COMRESET path -- an engine restart does not run ide_reset(),
>    so the drive's transfer state is preserved and the read still runs.
>
>    Patch 1 cancels the outstanding I/O when the command list is
>    unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
>    RESET already does from the same MMIO context. Patch 2 is a qtest
>    covering the PIO and DMA reply paths.
>
> 2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
>
>    A multi-sector PIO read whose byte-count limit spans more than one
>    sector must fetch the later sectors of a DRQ burst from inside the
>    completion of the first, asynchronous read. cd_read_sector_sync()
>    did that with a synchronous blk_pread(), which queues behind any
>    drain in progress. When a guest reset reaches
>    virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
>    read is in flight, the nested read is queued while the outer
>    completion still holds blk->in_flight, so the drain never finishes:
>    the main loop wedges in the drain with the BQL held and every
>    QMP/monitor operation blocks behind it.
>
>    Patch 8 reads the whole elementary transfer in a single asynchronous
>    request, so no read is ever issued mid-burst; cd_read_sector_sync()
>    is removed and the DMA path is unchanged. Patch 9 reproduces the
>    hang via x-blockdev-set-iothread, which takes the same
>    bdrv_drain_all_begin() path a guest reset does.
>
> Patches 3-7 are the test groundwork the deadlock fix needs: they fold
> the IDE CD-ROM read tests into one parametrized helper and add the
> previously missing coverage -- multi-sector DMA, and raw 2352-byte
> READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
> (including its in-place 2352 unpack) is exercised on both consumers.
> The test patches pass with or without patch 8, keeping the series
> bisectable.
>
> Patches 1-2 stand alone as the engine-restart crash fix; they are
> included here because the deadlock work shares and extends the same
> ATAPI read path and its test scaffolding.
>
> checkpatch is clean on all nine patches; the new and existing IDE and
> AHCI qtests pass.
>
> Denis V. Lunev (9):
>   hw/ide/ahci: cancel in-flight buffered reads on command engine restart
>   tests/qtest/ahci: test ATAPI read completing after engine restart
>   tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
>   tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
>   tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
>   tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
>   tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
>   hw/ide/atapi: read the whole elementary transfer asynchronously
>   tests/qtest/ahci: regression test for ATAPI read vs. drain
>
>  hw/ide/ahci.c             |   3 +
>  hw/ide/atapi.c            | 180 +++++++++++++++-----------------
>  tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
>  tests/qtest/libqos/ahci.c |   8 ++
>  tests/qtest/libqos/ahci.h |   2 +
>  6 files changed, 433 insertions(+), 172 deletions(-)
>
patches 1-2 landed via Philippe, thanks a lot.

What about the rest?

This is real obscure problem observable in production.

Thank you in advance,
    Den

Re: [PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Philippe Mathieu-Daudé 2 weeks, 2 days ago
On 9/7/26 17:22, Denis V. Lunev wrote:
> On 6/19/26 13:21, Denis V. Lunev wrote:
>> An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
>> the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
>> concurrent port operation, and it breaks in two distinct ways. Both are
>> guest-triggerable and reproduce on current master.
>>
>> Neither was found by inspection: both are real issues reported against
>> the qemu-kvm 10.0 downstream -- the crash from production, and the
>> deadlock from the downstream live-migration validation suite -- on
>> CD-ROMs backed by high-latency network storage, where a backend read
>> outlives the guest's SATA command timeout.
>>
>> 1. Command engine restart -> NULL-deref crash (patches 1-2)
>>
>>     A guest can stop and restart a port's command engine
>>     (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
>>     the command list and clears AHCIDevice.cur_cmd, but nothing tears
>>     down the outstanding read; its completion then dereferences the
>>     cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
>>     not the COMRESET path -- an engine restart does not run ide_reset(),
>>     so the drive's transfer state is preserved and the read still runs.
>>
>>     Patch 1 cancels the outstanding I/O when the command list is
>>     unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
>>     RESET already does from the same MMIO context. Patch 2 is a qtest
>>     covering the PIO and DMA reply paths.
>>
>> 2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
>>
>>     A multi-sector PIO read whose byte-count limit spans more than one
>>     sector must fetch the later sectors of a DRQ burst from inside the
>>     completion of the first, asynchronous read. cd_read_sector_sync()
>>     did that with a synchronous blk_pread(), which queues behind any
>>     drain in progress. When a guest reset reaches
>>     virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
>>     read is in flight, the nested read is queued while the outer
>>     completion still holds blk->in_flight, so the drain never finishes:
>>     the main loop wedges in the drain with the BQL held and every
>>     QMP/monitor operation blocks behind it.
>>
>>     Patch 8 reads the whole elementary transfer in a single asynchronous
>>     request, so no read is ever issued mid-burst; cd_read_sector_sync()
>>     is removed and the DMA path is unchanged. Patch 9 reproduces the
>>     hang via x-blockdev-set-iothread, which takes the same
>>     bdrv_drain_all_begin() path a guest reset does.
>>
>> Patches 3-7 are the test groundwork the deadlock fix needs: they fold
>> the IDE CD-ROM read tests into one parametrized helper and add the
>> previously missing coverage -- multi-sector DMA, and raw 2352-byte
>> READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
>> (including its in-place 2352 unpack) is exercised on both consumers.
>> The test patches pass with or without patch 8, keeping the series
>> bisectable.
>>
>> Patches 1-2 stand alone as the engine-restart crash fix; they are
>> included here because the deadlock work shares and extends the same
>> ATAPI read path and its test scaffolding.
>>
>> checkpatch is clean on all nine patches; the new and existing IDE and
>> AHCI qtests pass.
>>
>> Denis V. Lunev (9):
>>    hw/ide/ahci: cancel in-flight buffered reads on command engine restart
>>    tests/qtest/ahci: test ATAPI read completing after engine restart
>>    tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
>>    tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
>>    tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
>>    tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
>>    tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
>>    hw/ide/atapi: read the whole elementary transfer asynchronously
>>    tests/qtest/ahci: regression test for ATAPI read vs. drain
>>
>>   hw/ide/ahci.c             |   3 +
>>   hw/ide/atapi.c            | 180 +++++++++++++++-----------------
>>   tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
>>   tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
>>   tests/qtest/libqos/ahci.c |   8 ++
>>   tests/qtest/libqos/ahci.h |   2 +
>>   6 files changed, 433 insertions(+), 172 deletions(-)
>>
> patches 1-2 landed via Philippe, thanks a lot.
> 
> What about the rest?
> 
> This is real obscure problem observable in production.

Since IDE is listed as "Odd Fixes" and no listed maintainers
commented for a month on a fix, maybe you could step up as
a new one... My 2 cents ;)

Kevin, as core block/ maintainer, any objection?
Re: [PATCH 0/9] hw/ide: fix ATAPI CD reads racing port reset and drain
Posted by Denis V. Lunev 1 month ago
On 6/19/26 13:21, Denis V. Lunev wrote:
> An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
> the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
> concurrent port operation, and it breaks in two distinct ways. Both are
> guest-triggerable and reproduce on current master.
>
> Neither was found by inspection: both are real issues reported against
> the qemu-kvm 10.0 downstream -- the crash from production, and the
> deadlock from the downstream live-migration validation suite -- on
> CD-ROMs backed by high-latency network storage, where a backend read
> outlives the guest's SATA command timeout.
>
> 1. Command engine restart -> NULL-deref crash (patches 1-2)
>
>    A guest can stop and restart a port's command engine
>    (PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
>    the command list and clears AHCIDevice.cur_cmd, but nothing tears
>    down the outstanding read; its completion then dereferences the
>    cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
>    not the COMRESET path -- an engine restart does not run ide_reset(),
>    so the drive's transfer state is preserved and the read still runs.
>
>    Patch 1 cancels the outstanding I/O when the command list is
>    unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
>    RESET already does from the same MMIO context. Patch 2 is a qtest
>    covering the PIO and DMA reply paths.
>
> 2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
>
>    A multi-sector PIO read whose byte-count limit spans more than one
>    sector must fetch the later sectors of a DRQ burst from inside the
>    completion of the first, asynchronous read. cd_read_sector_sync()
>    did that with a synchronous blk_pread(), which queues behind any
>    drain in progress. When a guest reset reaches
>    virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
>    read is in flight, the nested read is queued while the outer
>    completion still holds blk->in_flight, so the drain never finishes:
>    the main loop wedges in the drain with the BQL held and every
>    QMP/monitor operation blocks behind it.
>
>    Patch 8 reads the whole elementary transfer in a single asynchronous
>    request, so no read is ever issued mid-burst; cd_read_sector_sync()
>    is removed and the DMA path is unchanged. Patch 9 reproduces the
>    hang via x-blockdev-set-iothread, which takes the same
>    bdrv_drain_all_begin() path a guest reset does.
>
> Patches 3-7 are the test groundwork the deadlock fix needs: they fold
> the IDE CD-ROM read tests into one parametrized helper and add the
> previously missing coverage -- multi-sector DMA, and raw 2352-byte
> READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
> (including its in-place 2352 unpack) is exercised on both consumers.
> The test patches pass with or without patch 8, keeping the series
> bisectable.
>
> Patches 1-2 stand alone as the engine-restart crash fix; they are
> included here because the deadlock work shares and extends the same
> ATAPI read path and its test scaffolding.
>
> checkpatch is clean on all nine patches; the new and existing IDE and
> AHCI qtests pass.
>
> Denis V. Lunev (9):
>   hw/ide/ahci: cancel in-flight buffered reads on command engine restart
>   tests/qtest/ahci: test ATAPI read completing after engine restart
>   tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
>   tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
>   tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
>   tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
>   tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
>   hw/ide/atapi: read the whole elementary transfer asynchronously
>   tests/qtest/ahci: regression test for ATAPI read vs. drain
>
>  hw/ide/ahci.c             |   3 +
>  hw/ide/atapi.c            | 180 +++++++++++++++-----------------
>  tests/qtest/ahci-test.c   | 201 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/ide-test.c    | 211 ++++++++++++++++++++++++--------------
>  tests/qtest/libqos/ahci.c |   8 ++
>  tests/qtest/libqos/ahci.h |   2 +
>  6 files changed, 433 insertions(+), 172 deletions(-)
>
ping