From: Thomas Huth <thuth@redhat.com>
ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
two IDE drives on a bus (primary or secondary drive) to cancel all
pending DMA transfers on the drive. The code then checks
s->bus->dma->aiocb to see whether there is any IO in flight on the
*bus* and then calls blk_drain(s->blk) to wait for its completion.
However, s->bus->dma->aiocb might belong to the other drive on the
bus, and if there is no disk attached to the current drive, s->blk
is NULL. Since blk_drain() does not check its parameter for a NULL
pointer, QEMU can crash in such a case.
To fix the problem, we have to check that "blk" is not NULL before
calling blk_drain(). And we have to call blk_drain() for both drives,
otherwise the assert(s->bus->dma->aiocb == NULL) statement after
the blk_drain() might trigger if the IO in flight belongs to the
the other drive.
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/905
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4052
Reported-by: dong ling
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
v2: Drain both drives
hw/ide/core.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f78b00220b8..f43ab95b07b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
* In the future we'll be able to safely cancel the I/O if the
* whole DMA operation will be submitted to disk with a single
* aio operation with preadv/pwritev.
+ *
+ * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
+ * so we have to drain both drives to get it cleared.
*/
if (s->bus->dma->aiocb) {
trace_ide_cancel_dma_sync_remaining();
- blk_drain(s->blk);
+ for (int i = 0; i < 2; i++) {
+ if (s->bus->ifs[i].blk) {
+ blk_drain(s->bus->ifs[i].blk);
+ }
+ }
assert(s->bus->dma->aiocb == NULL);
}
}
--
2.55.0On 21/07/2026 08:02, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
>
> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
> two IDE drives on a bus (primary or secondary drive) to cancel all
> pending DMA transfers on the drive. The code then checks
> s->bus->dma->aiocb to see whether there is any IO in flight on the
> *bus* and then calls blk_drain(s->blk) to wait for its completion.
> However, s->bus->dma->aiocb might belong to the other drive on the
> bus, and if there is no disk attached to the current drive, s->blk
> is NULL. Since blk_drain() does not check its parameter for a NULL
> pointer, QEMU can crash in such a case.
>
> To fix the problem, we have to check that "blk" is not NULL before
> calling blk_drain(). And we have to call blk_drain() for both drives,
> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
> the blk_drain() might trigger if the IO in flight belongs to the
> the other drive.
>
> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
> Reported-by: dong ling
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2: Drain both drives
>
> hw/ide/core.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f78b00220b8..f43ab95b07b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
> * In the future we'll be able to safely cancel the I/O if the
> * whole DMA operation will be submitted to disk with a single
> * aio operation with preadv/pwritev.
> + *
> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
> + * so we have to drain both drives to get it cleared.
> */
> if (s->bus->dma->aiocb) {
> trace_ide_cancel_dma_sync_remaining();
> - blk_drain(s->blk);
> + for (int i = 0; i < 2; i++) {
> + if (s->bus->ifs[i].blk) {
> + blk_drain(s->bus->ifs[i].blk);
> + }
> + }
> assert(s->bus->dma->aiocb == NULL);
> }
> }
Doesn't ide_bus_active_if() do the right thing here for in-flight
requests? I'd expect to be able to do something like:
if (s->bus->dma->aiocb) {
trace_ide_cancel_dma_sync_remaining();
blk_drain(ide_bus_active_if(s->bus)->blk);
}
ATB,
Mark.
On 21/7/26 10:29, Mark Cave-Ayland wrote:
> On 21/07/2026 08:02, Thomas Huth wrote:
>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>> two IDE drives on a bus (primary or secondary drive) to cancel all
>> pending DMA transfers on the drive. The code then checks
>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>> However, s->bus->dma->aiocb might belong to the other drive on the
>> bus, and if there is no disk attached to the current drive, s->blk
>> is NULL. Since blk_drain() does not check its parameter for a NULL
>> pointer, QEMU can crash in such a case.
>>
>> To fix the problem, we have to check that "blk" is not NULL before
>> calling blk_drain(). And we have to call blk_drain() for both drives,
>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>> the blk_drain() might trigger if the IO in flight belongs to the
>> the other drive.
>>
>> Resolves: https://urldefense.proofpoint.com/v2/url?
>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>> Resolves: https://urldefense.proofpoint.com/v2/url?
>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>> Reported-by: dong ling
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> v2: Drain both drives
>>
>> hw/ide/core.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index f78b00220b8..f43ab95b07b 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>> * In the future we'll be able to safely cancel the I/O if the
>> * whole DMA operation will be submitted to disk with a single
>> * aio operation with preadv/pwritev.
>> + *
>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>> + * so we have to drain both drives to get it cleared.
>> */
>> if (s->bus->dma->aiocb) {
>> trace_ide_cancel_dma_sync_remaining();
>> - blk_drain(s->blk);
>> + for (int i = 0; i < 2; i++) {
>> + if (s->bus->ifs[i].blk) {
>> + blk_drain(s->bus->ifs[i].blk);
>> + }
>> + }
>> assert(s->bus->dma->aiocb == NULL);
>> }
>> }
>
> Doesn't ide_bus_active_if() do the right thing here for in-flight
> requests?
bmdma_cmd_writeb() already calls ide_bus_active_if().
Is the issue in the caller, bmdma_cmd_writeb(), instead of
here in ide_cancel_dma_sync()?
> I'd expect to be able to do something like:
>
> if (s->bus->dma->aiocb) {
> trace_ide_cancel_dma_sync_remaining();
> blk_drain(ide_bus_active_if(s->bus)->blk);
> }
>
>
> ATB,
>
> Mark.
>
>
On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>> On 21/07/2026 08:02, Thomas Huth wrote:
>>
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>> pending DMA transfers on the drive. The code then checks
>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>> bus, and if there is no disk attached to the current drive, s->blk
>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>> pointer, QEMU can crash in such a case.
>>>
>>> To fix the problem, we have to check that "blk" is not NULL before
>>> calling blk_drain(). And we have to call blk_drain() for both drives,
>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>> the blk_drain() might trigger if the IO in flight belongs to the
>>> the other drive.
>>>
>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>> Reported-by: dong ling
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> v2: Drain both drives
>>>
>>> hw/ide/core.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index f78b00220b8..f43ab95b07b 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>> * In the future we'll be able to safely cancel the I/O if the
>>> * whole DMA operation will be submitted to disk with a single
>>> * aio operation with preadv/pwritev.
>>> + *
>>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>>> + * so we have to drain both drives to get it cleared.
>>> */
>>> if (s->bus->dma->aiocb) {
>>> trace_ide_cancel_dma_sync_remaining();
>>> - blk_drain(s->blk);
>>> + for (int i = 0; i < 2; i++) {
>>> + if (s->bus->ifs[i].blk) {
>>> + blk_drain(s->bus->ifs[i].blk);
>>> + }
>>> + }
>>> assert(s->bus->dma->aiocb == NULL);
>>> }
>>> }
>>
>> Doesn't ide_bus_active_if() do the right thing here for in-flight
>> requests?
>
> bmdma_cmd_writeb() already calls ide_bus_active_if().
>
> Is the issue in the caller, bmdma_cmd_writeb(), instead of
> here in ide_cancel_dma_sync()?
i.e.:
-- >8 --
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 7ce1ae67ab2..e7c6af9a49e 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
/* Ignore writes to SSBM if it keeps the old value */
if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
if (!(val & BM_CMD_START)) {
- ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
+ ide_cancel_dma_sync(bmdma_active_if(bm));
bm->status &= ~BM_STATUS_DMAING;
} else {
bm->cur_addr = bm->addr;
---
>
>> I'd expect to be able to do something like:
>>
>> if (s->bus->dma->aiocb) {
>> trace_ide_cancel_dma_sync_remaining();
>> blk_drain(ide_bus_active_if(s->bus)->blk);
>> }
>>
>>
>> ATB,
>>
>> Mark.
>>
>>
>
>
On 21/07/2026 11.58, Philippe Mathieu-Daudé wrote:
> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>> pending DMA transfers on the drive. The code then checks
>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>> pointer, QEMU can crash in such a case.
>>>>
>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>> calling blk_drain(). And we have to call blk_drain() for both drives,
>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>> the other drive.
>>>>
>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>> Reported-by: dong ling
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> v2: Drain both drives
>>>>
>>>> hw/ide/core.c | 9 ++++++++-
>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index f78b00220b8..f43ab95b07b 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>> * In the future we'll be able to safely cancel the I/O if the
>>>> * whole DMA operation will be submitted to disk with a single
>>>> * aio operation with preadv/pwritev.
>>>> + *
>>>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>>>> + * so we have to drain both drives to get it cleared.
>>>> */
>>>> if (s->bus->dma->aiocb) {
>>>> trace_ide_cancel_dma_sync_remaining();
>>>> - blk_drain(s->blk);
>>>> + for (int i = 0; i < 2; i++) {
>>>> + if (s->bus->ifs[i].blk) {
>>>> + blk_drain(s->bus->ifs[i].blk);
>>>> + }
>>>> + }
>>>> assert(s->bus->dma->aiocb == NULL);
>>>> }
>>>> }
>>>
>>> Doesn't ide_bus_active_if() do the right thing here for in-flight requests?
>>
>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>
>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>> here in ide_cancel_dma_sync()?
>
> i.e.:
>
> -- >8 --
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 7ce1ae67ab2..e7c6af9a49e 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
> /* Ignore writes to SSBM if it keeps the old value */
> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
> if (!(val & BM_CMD_START)) {
> - ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
> + ide_cancel_dma_sync(bmdma_active_if(bm));
> bm->status &= ~BM_STATUS_DMAING;
> } else {
> bm->cur_addr = bm->addr;
No, I don't think this will fix all cases. While
https://gitlab.com/qemu-project/qemu/-/work_items/905 has a call trace with
bmdma_cmd_writeb() in it,
https://gitlab.com/qemu-project/qemu/-/work_items/4052 is slightly
different: The backtrace shows ide_bus_perform_srst() -> ide_perform_srst()
-> ide_cancel_dma_sync() here instead. I'd say let's play safe and always
drain both interfaces. WDYT?
Thomas
On 21/7/26 12:21, Thomas Huth wrote:
> On 21/07/2026 11.58, Philippe Mathieu-Daudé wrote:
>> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>>
>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>
>>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>>> pending DMA transfers on the drive. The code then checks
>>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>>> pointer, QEMU can crash in such a case.
>>>>>
>>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>>> calling blk_drain(). And we have to call blk_drain() for both drives,
>>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>>> the other drive.
>>>>>
>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>>> Reported-by: dong ling
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>> v2: Drain both drives
>>>>>
>>>>> hw/ide/core.c | 9 ++++++++-
>>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>> index f78b00220b8..f43ab95b07b 100644
>>>>> --- a/hw/ide/core.c
>>>>> +++ b/hw/ide/core.c
>>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>>> * In the future we'll be able to safely cancel the I/O if the
>>>>> * whole DMA operation will be submitted to disk with a single
>>>>> * aio operation with preadv/pwritev.
>>>>> + *
>>>>> + * Note: s->bus->dma->aiocb might belong to the adjacent
>>>>> IDEState,
>>>>> + * so we have to drain both drives to get it cleared.
>>>>> */
>>>>> if (s->bus->dma->aiocb) {
>>>>> trace_ide_cancel_dma_sync_remaining();
>>>>> - blk_drain(s->blk);
>>>>> + for (int i = 0; i < 2; i++) {
>>>>> + if (s->bus->ifs[i].blk) {
>>>>> + blk_drain(s->bus->ifs[i].blk);
>>>>> + }
>>>>> + }
>>>>> assert(s->bus->dma->aiocb == NULL);
>>>>> }
>>>>> }
>>>>
>>>> Doesn't ide_bus_active_if() do the right thing here for in-flight
>>>> requests?
>>>
>>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>>
>>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>>> here in ide_cancel_dma_sync()?
>>
>> i.e.:
>>
>> -- >8 --
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 7ce1ae67ab2..e7c6af9a49e 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>> /* Ignore writes to SSBM if it keeps the old value */
>> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>> if (!(val & BM_CMD_START)) {
>> - ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
>> + ide_cancel_dma_sync(bmdma_active_if(bm));
>> bm->status &= ~BM_STATUS_DMAING;
>> } else {
>> bm->cur_addr = bm->addr;
> No, I don't think this will fix all cases. While https://gitlab.com/
> qemu-project/qemu/-/work_items/905 has a call trace with
> bmdma_cmd_writeb() in it, https://gitlab.com/qemu-project/qemu/-/
> work_items/4052 is slightly different: The backtrace shows
> ide_bus_perform_srst() -> ide_perform_srst() -> ide_cancel_dma_sync()
> here instead. I'd say let's play safe and always drain both interfaces.
> WDYT?
I dunno I'm kind of lost. Can't blk_bs() handle NULL blk?
-- >8 --
diff --git a/block/block-backend.c b/block/block-backend.c
index 37ba7e9fc40..b5d84c9789a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -760,7 +760,7 @@ BlockBackend *blk_by_name(const char *name)
BlockDriverState *blk_bs(BlockBackend *blk)
{
IO_CODE();
- return blk->root ? blk->root->bs : NULL;
+ return (blk && blk->root) ? blk->root->bs : NULL;
}
---
On 21/07/2026 12.55, Philippe Mathieu-Daudé wrote:
> On 21/7/26 12:21, Thomas Huth wrote:
>> On 21/07/2026 11.58, Philippe Mathieu-Daudé wrote:
>>> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>>>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>>>
>>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>>
>>>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>>>> pending DMA transfers on the drive. The code then checks
>>>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>>>> pointer, QEMU can crash in such a case.
>>>>>>
>>>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>>>> calling blk_drain(). And we have to call blk_drain() for both drives,
>>>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>>>> the other drive.
>>>>>>
>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>>>> Reported-by: dong ling
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>> v2: Drain both drives
>>>>>>
>>>>>> hw/ide/core.c | 9 ++++++++-
>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>>> index f78b00220b8..f43ab95b07b 100644
>>>>>> --- a/hw/ide/core.c
>>>>>> +++ b/hw/ide/core.c
>>>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>>>> * In the future we'll be able to safely cancel the I/O if the
>>>>>> * whole DMA operation will be submitted to disk with a single
>>>>>> * aio operation with preadv/pwritev.
>>>>>> + *
>>>>>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>>>>>> + * so we have to drain both drives to get it cleared.
>>>>>> */
>>>>>> if (s->bus->dma->aiocb) {
>>>>>> trace_ide_cancel_dma_sync_remaining();
>>>>>> - blk_drain(s->blk);
>>>>>> + for (int i = 0; i < 2; i++) {
>>>>>> + if (s->bus->ifs[i].blk) {
>>>>>> + blk_drain(s->bus->ifs[i].blk);
>>>>>> + }
>>>>>> + }
>>>>>> assert(s->bus->dma->aiocb == NULL);
>>>>>> }
>>>>>> }
>>>>>
>>>>> Doesn't ide_bus_active_if() do the right thing here for in-flight
>>>>> requests?
>>>>
>>>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>>>
>>>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>>>> here in ide_cancel_dma_sync()?
>>>
>>> i.e.:
>>>
>>> -- >8 --
>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>> index 7ce1ae67ab2..e7c6af9a49e 100644
>>> --- a/hw/ide/pci.c
>>> +++ b/hw/ide/pci.c
>>> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>>> /* Ignore writes to SSBM if it keeps the old value */
>>> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>>> if (!(val & BM_CMD_START)) {
>>> - ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
>>> + ide_cancel_dma_sync(bmdma_active_if(bm));
>>> bm->status &= ~BM_STATUS_DMAING;
>>> } else {
>>> bm->cur_addr = bm->addr;
>> No, I don't think this will fix all cases. While https://gitlab.com/ qemu-
>> project/qemu/-/work_items/905 has a call trace with bmdma_cmd_writeb() in
>> it, https://gitlab.com/qemu-project/qemu/-/ work_items/4052 is slightly
>> different: The backtrace shows ide_bus_perform_srst() ->
>> ide_perform_srst() -> ide_cancel_dma_sync() here instead. I'd say let's
>> play safe and always drain both interfaces. WDYT?
>
> I dunno I'm kind of lost. Can't blk_bs() handle NULL blk?
>
> -- >8 --
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 37ba7e9fc40..b5d84c9789a 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -760,7 +760,7 @@ BlockBackend *blk_by_name(const char *name)
> BlockDriverState *blk_bs(BlockBackend *blk)
> {
> IO_CODE();
> - return blk->root ? blk->root->bs : NULL;
> + return (blk && blk->root) ? blk->root->bs : NULL;
> }
That would maybe help with the NULL pointer crash, but it would not prevent
the assert(s->bus->dma->aiocb == NULL) from triggering.
Simply draining both drives seems to be the only solution that I see right
now, unless we want to remove the assert() statement?
Thomas
On 21/7/26 13:08, Thomas Huth wrote:
> On 21/07/2026 12.55, Philippe Mathieu-Daudé wrote:
>> On 21/7/26 12:21, Thomas Huth wrote:
>>> On 21/07/2026 11.58, Philippe Mathieu-Daudé wrote:
>>>> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>>>>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>>>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>>>>
>>>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>>>
>>>>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>>>>> pending DMA transfers on the drive. The code then checks
>>>>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>>>>> pointer, QEMU can crash in such a case.
>>>>>>>
>>>>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>>>>> calling blk_drain(). And we have to call blk_drain() for both
>>>>>>> drives,
>>>>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>>>>> the other drive.
>>>>>>>
>>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>>>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>>>>> Reported-by: dong ling
>>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>>> ---
>>>>>>> v2: Drain both drives
>>>>>>>
>>>>>>> hw/ide/core.c | 9 ++++++++-
>>>>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>>>> index f78b00220b8..f43ab95b07b 100644
>>>>>>> --- a/hw/ide/core.c
>>>>>>> +++ b/hw/ide/core.c
>>>>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>>>>> * In the future we'll be able to safely cancel the I/O if the
>>>>>>> * whole DMA operation will be submitted to disk with a single
>>>>>>> * aio operation with preadv/pwritev.
>>>>>>> + *
>>>>>>> + * Note: s->bus->dma->aiocb might belong to the adjacent
>>>>>>> IDEState,
>>>>>>> + * so we have to drain both drives to get it cleared.
>>>>>>> */
>>>>>>> if (s->bus->dma->aiocb) {
>>>>>>> trace_ide_cancel_dma_sync_remaining();
>>>>>>> - blk_drain(s->blk);
>>>>>>> + for (int i = 0; i < 2; i++) {
>>>>>>> + if (s->bus->ifs[i].blk) {
>>>>>>> + blk_drain(s->bus->ifs[i].blk);
>>>>>>> + }
>>>>>>> + }
>>>>>>> assert(s->bus->dma->aiocb == NULL);
>>>>>>> }
>>>>>>> }
>>>>>>
>>>>>> Doesn't ide_bus_active_if() do the right thing here for in-flight
>>>>>> requests?
>>>>>
>>>>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>>>>
>>>>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>>>>> here in ide_cancel_dma_sync()?
>>>>
>>>> i.e.:
>>>>
>>>> -- >8 --
>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>> index 7ce1ae67ab2..e7c6af9a49e 100644
>>>> --- a/hw/ide/pci.c
>>>> +++ b/hw/ide/pci.c
>>>> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>>>> /* Ignore writes to SSBM if it keeps the old value */
>>>> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>>>> if (!(val & BM_CMD_START)) {
>>>> - ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
>>>> + ide_cancel_dma_sync(bmdma_active_if(bm));
>>>> bm->status &= ~BM_STATUS_DMAING;
>>>> } else {
>>>> bm->cur_addr = bm->addr;
>>> No, I don't think this will fix all cases. While https://gitlab.com/
>>> qemu- project/qemu/-/work_items/905 has a call trace with
>>> bmdma_cmd_writeb() in it, https://gitlab.com/qemu-project/qemu/-/
>>> work_items/4052 is slightly different: The backtrace shows
>>> ide_bus_perform_srst() -> ide_perform_srst() -> ide_cancel_dma_sync()
>>> here instead. I'd say let's play safe and always drain both
>>> interfaces. WDYT?
>>
>> I dunno I'm kind of lost. Can't blk_bs() handle NULL blk?
>>
>> -- >8 --
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 37ba7e9fc40..b5d84c9789a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -760,7 +760,7 @@ BlockBackend *blk_by_name(const char *name)
>> BlockDriverState *blk_bs(BlockBackend *blk)
>> {
>> IO_CODE();
>> - return blk->root ? blk->root->bs : NULL;
>> + return (blk && blk->root) ? blk->root->bs : NULL;
>> }
> That would maybe help with the NULL pointer crash, but it would not
> prevent the assert(s->bus->dma->aiocb == NULL) from triggering.
>
> Simply draining both drives seems to be the only solution that I see
> right now, unless we want to remove the assert() statement?
OK... Maybe move the comment before the for() for clarity.
Still some review from qemu-block@ team would help...
>
> Thomas
>
>
On 21/7/26 11:58, Philippe Mathieu-Daudé wrote:
> On 21/7/26 11:51, Philippe Mathieu-Daudé wrote:
>> On 21/7/26 10:29, Mark Cave-Ayland wrote:
>>> On 21/07/2026 08:02, Thomas Huth wrote:
>>>
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>>>> two IDE drives on a bus (primary or secondary drive) to cancel all
>>>> pending DMA transfers on the drive. The code then checks
>>>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>>>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>>>> However, s->bus->dma->aiocb might belong to the other drive on the
>>>> bus, and if there is no disk attached to the current drive, s->blk
>>>> is NULL. Since blk_drain() does not check its parameter for a NULL
>>>> pointer, QEMU can crash in such a case.
>>>>
>>>> To fix the problem, we have to check that "blk" is not NULL before
>>>> calling blk_drain(). And we have to call blk_drain() for both drives,
>>>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>>>> the blk_drain() might trigger if the IO in flight belongs to the
>>>> the other drive.
>>>>
>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>>> Resolves: https://urldefense.proofpoint.com/v2/url?
>>>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>>>> Reported-by: dong ling
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> v2: Drain both drives
>>>>
>>>> hw/ide/core.c | 9 ++++++++-
>>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index f78b00220b8..f43ab95b07b 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>>>> * In the future we'll be able to safely cancel the I/O if the
>>>> * whole DMA operation will be submitted to disk with a single
>>>> * aio operation with preadv/pwritev.
>>>> + *
>>>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>>>> + * so we have to drain both drives to get it cleared.
>>>> */
>>>> if (s->bus->dma->aiocb) {
>>>> trace_ide_cancel_dma_sync_remaining();
>>>> - blk_drain(s->blk);
>>>> + for (int i = 0; i < 2; i++) {
>>>> + if (s->bus->ifs[i].blk) {
>>>> + blk_drain(s->bus->ifs[i].blk);
>>>> + }
>>>> + }
>>>> assert(s->bus->dma->aiocb == NULL);
>>>> }
>>>> }
>>>
>>> Doesn't ide_bus_active_if() do the right thing here for in-flight
>>> requests?
>>
>> bmdma_cmd_writeb() already calls ide_bus_active_if().
>>
>> Is the issue in the caller, bmdma_cmd_writeb(), instead of
>> here in ide_cancel_dma_sync()?
>
> i.e.:
>
> -- >8 --
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 7ce1ae67ab2..e7c6af9a49e 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -384,7 +384,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
> /* Ignore writes to SSBM if it keeps the old value */
> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
> if (!(val & BM_CMD_START)) {
Also, shouldn't be calling ide_transfer_halt() first?
(Without IDE expert it seems late to get a fix for the release...)
> - ide_cancel_dma_sync(ide_bus_active_if(bm->bus));
> + ide_cancel_dma_sync(bmdma_active_if(bm));
> bm->status &= ~BM_STATUS_DMAING;
> } else {
> bm->cur_addr = bm->addr;
> ---
>
>>
>>> I'd expect to be able to do something like:
>>>
>>> if (s->bus->dma->aiocb) {
>>> trace_ide_cancel_dma_sync_remaining();
>>> blk_drain(ide_bus_active_if(s->bus)->blk);
>>> }
>>>
>>>
>>> ATB,
>>>
>>> Mark.
>>>
>>>
>>
>>
>
>
On 21/07/2026 10.29, Mark Cave-Ayland wrote:
> On 21/07/2026 08:02, Thomas Huth wrote:
>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> ide_cancel_dma_sync() is called with a "IDEState *s" for one of the
>> two IDE drives on a bus (primary or secondary drive) to cancel all
>> pending DMA transfers on the drive. The code then checks
>> s->bus->dma->aiocb to see whether there is any IO in flight on the
>> *bus* and then calls blk_drain(s->blk) to wait for its completion.
>> However, s->bus->dma->aiocb might belong to the other drive on the
>> bus, and if there is no disk attached to the current drive, s->blk
>> is NULL. Since blk_drain() does not check its parameter for a NULL
>> pointer, QEMU can crash in such a case.
>>
>> To fix the problem, we have to check that "blk" is not NULL before
>> calling blk_drain(). And we have to call blk_drain() for both drives,
>> otherwise the assert(s->bus->dma->aiocb == NULL) statement after
>> the blk_drain() might trigger if the IO in flight belongs to the
>> the other drive.
>>
>> Resolves: https://urldefense.proofpoint.com/v2/url?
>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_905&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=rCgSz13wVfk11Z-Uw5WTQViAQKkSvwpQqf_dJX1tiro&e=
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>> Resolves: https://urldefense.proofpoint.com/v2/url?
>> u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_work-5Fitems_4052&d=DwIDAg&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=BbZFP82dEZMyWzN5GnJ9XaxwmyrmYGbqWbEBNSUM_afB5JRRF3InIaZAr5UpUUh2&s=B-AkqMAsQwA0LzU5ptwPJ3ZIBAA8ncf5ZbQg8GTwaME&e=
>> Reported-by: dong ling
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> v2: Drain both drives
>>
>> hw/ide/core.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index f78b00220b8..f43ab95b07b 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -741,10 +741,17 @@ void ide_cancel_dma_sync(IDEState *s)
>> * In the future we'll be able to safely cancel the I/O if the
>> * whole DMA operation will be submitted to disk with a single
>> * aio operation with preadv/pwritev.
>> + *
>> + * Note: s->bus->dma->aiocb might belong to the adjacent IDEState,
>> + * so we have to drain both drives to get it cleared.
>> */
>> if (s->bus->dma->aiocb) {
>> trace_ide_cancel_dma_sync_remaining();
>> - blk_drain(s->blk);
>> + for (int i = 0; i < 2; i++) {
>> + if (s->bus->ifs[i].blk) {
>> + blk_drain(s->bus->ifs[i].blk);
>> + }
>> + }
>> assert(s->bus->dma->aiocb == NULL);
>> }
>> }
>
> Doesn't ide_bus_active_if() do the right thing here for in-flight requests?
> I'd expect to be able to do something like:
>
> if (s->bus->dma->aiocb) {
> trace_ide_cancel_dma_sync_remaining();
> blk_drain(ide_bus_active_if(s->bus)->blk);
> }
Sounds appealing, so I gave it a try, but it does not seem to work: I can
still trigger the problem with this code (both, the NULL pointer dereference
and the assert() after the drain). Looks like it is possible to trigger a
DMA transfer for the other drive, and then while the transfer is still in
flight, switch the current drive and trigger the code path that calls
ide_cancel_dma_sync(). So we need indeed to drain both drives, i.e. use this
current patch here.
Thomas
© 2016 - 2026 Red Hat, Inc.