On 7/7/26 15:15, Philippe Mathieu-Daudé wrote:
> Hi Denis,
>
> On 7/7/26 15:02, Denis V. Lunev wrote:
>> On 7/6/26 09:21, Philippe Mathieu-Daudé wrote:
>>> Hi Denis,
>>>
>>> On 19/6/26 13:21, Denis V. Lunev via qemu development wrote:
>>>> ATAPI CD reads are issued through ide_buffered_readv()
>>>> (cd_read_sector() and ide_atapi_cmd_read_dma_cb() in hw/ide/atapi.c).
>>>> The PIO path discards the returned aiocb; the DMA path stores it in
>>>> s->bus->dma->aiocb.
>>>>
>>>> A guest can stop and restart a port's command engine
>>>> (PxCMD.ST 1 -> 0 -> 1) while such a read is still in flight. Stopping
>>>> the engine unmaps the command list (ahci_unmap_clb_address()) and
>>>> restarting it re-maps the list and clears AHCIDevice.cur_cmd to NULL,
>>>> but nothing tears down the outstanding read. This path does not run
>>>> ide_reset(), so the drive's transfer state is preserved and the read
>>>> still completes. Its callbacks then dereference the stale or NULL
>>>> cur_cmd in the AHCI transfer helpers:
>>>>
>>>> PIO: cd_read_sector_cb() -> ide_atapi_cmd_reply_end() ->
>>>> ide_transfer_start_norecurse() -> ahci_pio_transfer()
>>>>
>>>> DMA: ide_atapi_cmd_read_dma_cb() -> ahci_dma_rw_buf() ->
>>>> ahci_populate_sglist()
>>>>
>>>> Both crash with a NULL cur_cmd; the PIO variant has been seen in the
>>>> field.
>>>>
>>>> Cancel the outstanding I/O when the command list is unmapped, reusing
>>>> ide_cancel_dma_sync() as the ATAPI DEVICE RESET command does. It runs
>>>> the completion callback with -ECANCELED (which tears down
>>>> s->bus->dma->aiocb for the DMA case) and orphans the buffered request,
>>>> so the eventual asynchronous completion is a no-op. Merely setting the
>>>> orphaned flag is not enough: it would leave s->bus->dma->aiocb pointing
>>>> at a freed aiocb that a later reset would cancel.
>>>>
>>>> Fixes: 1d8c11d63154 ("ide: add support for IDEBufferedRequest")
>>>> Signed-off-by: Denis V. Lunev <[email protected]>
>>>> ---
>>>> hw/ide/ahci.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>> index c2b4432b94..8e3684cd3c 100644
>>>> --- a/hw/ide/ahci.c
>>>> +++ b/hw/ide/ahci.c
>>>> @@ -740,6 +740,9 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
>>>>
>>>> static void ahci_unmap_clb_address(AHCIDevice *ad)
>>>> {
>>>> + /* Cancel in-flight reads that would complete against a cleared
>>>> cur_cmd. */
>>>> + ide_cancel_dma_sync(&ad->port.ifs[0]);
>>> ide_bus_active_if(&ad->port.ifs) would be safer, otherwise the
>>> patch looks reasonable.
>> Anything else for the series?
>>
>> Should I respin or this change could be cooked at merge?
> I took this patch via my hw-misc tree (using ide_bus_active_if) because
> it I could understand it and it looked trivial enough to me; however I
> don't have enough knowledge (don't feel confident at all) to look at the
> rest, I'm sorry :|
>
> Regards,
>
> Phil.
In that case you should also take next one - this is unit test
for the fix.
Den
