[Qemu-devel] ahci: crash after duplicate bh registration

[Qemu-devel] ahci: crash after duplicate bh registration

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Hi Alex,

I've seen crashes caused by ahci_check_cmd_bh unregistering a NULL bh.
It looks like ahci_dma_set_inactive can a called while there is already
a bh hanging around. Patch below cures the issue, but I have no clue if
such an invocation order is valid at all.

Jan

---

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e2ed2ad..7870030 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1066,9 +1066,11 @@ static int ahci_dma_set_inactive(IDEDMA *dma)
 
     ad->dma_cb = NULL;
 
-    /* maybe we still have something to process, check later */
-    ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
-    qemu_bh_schedule(ad->check_bh);
+    if (!ad->check_bh) {
+        /* maybe we still have something to process, check later */
+        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+        qemu_bh_schedule(ad->check_bh);
+    }
 
     return 0;
 }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] ahci: crash after duplicate bh registration

[Alexander Graf]

On 05/08/2011 09:10 PM, Jan Kiszka wrote:
> Hi Alex,
>
> I've seen crashes caused by ahci_check_cmd_bh unregistering a NULL bh.
> It looks like ahci_dma_set_inactive can a called while there is already
> a bh hanging around. Patch below cures the issue, but I have no clue if
> such an invocation order is valid at all.

It's certainly guest triggerable, so yes, let's check here.

Acked-by: Alexander Graf <agraf@suse.de>


Alex

> Jan
>
> ---
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index e2ed2ad..7870030 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1066,9 +1066,11 @@ static int ahci_dma_set_inactive(IDEDMA *dma)
>
>       ad->dma_cb = NULL;
>
> -    /* maybe we still have something to process, check later */
> -    ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
> -    qemu_bh_schedule(ad->check_bh);
> +    if (!ad->check_bh) {
> +        /* maybe we still have something to process, check later */
> +        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
> +        qemu_bh_schedule(ad->check_bh);
> +    }
>
>       return 0;
>   }
>

Re: [Qemu-devel] ahci: crash after duplicate bh registration

[Kevin Wolf]

Am 09.05.2011 16:12, schrieb Alexander Graf:
> On 05/08/2011 09:10 PM, Jan Kiszka wrote:
>> Hi Alex,
>>
>> I've seen crashes caused by ahci_check_cmd_bh unregistering a NULL bh.
>> It looks like ahci_dma_set_inactive can a called while there is already
>> a bh hanging around. Patch below cures the issue, but I have no clue if
>> such an invocation order is valid at all.
> 
> It's certainly guest triggerable, so yes, let's check here.
> 
> Acked-by: Alexander Graf <agraf@suse.de>

Yes, the change makes sense to me. Please resend this as a proper patch,
Jan.

However, I still think Jan's question is valid: Is the AHCI emulation
supposed to run multiple DMA requests at once using the core.c
functions? I'd find it surprising if this actually worked well.

Kevin

Re: [Qemu-devel] ahci: crash after duplicate bh registration

[Alexander Graf]

On 05/09/2011 04:26 PM, Kevin Wolf wrote:
> Am 09.05.2011 16:12, schrieb Alexander Graf:
>> On 05/08/2011 09:10 PM, Jan Kiszka wrote:
>>> Hi Alex,
>>>
>>> I've seen crashes caused by ahci_check_cmd_bh unregistering a NULL bh.
>>> It looks like ahci_dma_set_inactive can a called while there is already
>>> a bh hanging around. Patch below cures the issue, but I have no clue if
>>> such an invocation order is valid at all.
>> It's certainly guest triggerable, so yes, let's check here.
>>
>> Acked-by: Alexander Graf<agraf@suse.de>
> Yes, the change makes sense to me. Please resend this as a proper patch,
> Jan.
>
> However, I still think Jan's question is valid: Is the AHCI emulation
> supposed to run multiple DMA requests at once using the core.c
> functions? I'd find it surprising if this actually worked well.

Not through the IDE core, no. There it can process a queue of IDE 
commands after each other or do NCQ, but that goes a different code 
patch, can do multiple requests at once though.

I'm not sure how this got triggered.


Alex

Re: [Qemu-devel] ahci: crash after duplicate bh registration

[Jan Kiszka]

On 2011-05-09 16:31, Alexander Graf wrote:
> On 05/09/2011 04:26 PM, Kevin Wolf wrote:
>> Am 09.05.2011 16:12, schrieb Alexander Graf:
>>> On 05/08/2011 09:10 PM, Jan Kiszka wrote:
>>>> Hi Alex,
>>>>
>>>> I've seen crashes caused by ahci_check_cmd_bh unregistering a NULL bh.
>>>> It looks like ahci_dma_set_inactive can a called while there is already
>>>> a bh hanging around. Patch below cures the issue, but I have no clue if
>>>> such an invocation order is valid at all.
>>> It's certainly guest triggerable, so yes, let's check here.
>>>
>>> Acked-by: Alexander Graf<agraf@suse.de>
>> Yes, the change makes sense to me. Please resend this as a proper patch,
>> Jan.

Will do.

>>
>> However, I still think Jan's question is valid: Is the AHCI emulation
>> supposed to run multiple DMA requests at once using the core.c
>> functions? I'd find it surprising if this actually worked well.
> 
> Not through the IDE core, no. There it can process a queue of IDE
> commands after each other or do NCQ, but that goes a different code
> patch, can do multiple requests at once though.
> 
> I'm not sure how this got triggered.

Forgot to mention: With a hacked-up q35 series. I may have broken
something there, or it was already broken (there are definitely bugs in
that series), so upstream might not expose the problem at all.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux