Re: [RESEND PATCH] hw/dma: fix crash caused by race condition

[David Hildenbrand]

On 01.06.22 02:20, Tong Zhang wrote:
> Hi David,
> 
> On Mon, May 30, 2022 at 9:19 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 27.04.22 22:51, Tong Zhang wrote:
>>> assert(dbs->acb) is meant to check the return value of io_func per
>>> documented in commit 6bee44ea34 ("dma: the passed io_func does not
>>> return NULL"). However, there is a chance that after calling
>>> aio_context_release(dbs->ctx); the dma_blk_cb function is called before
>>> the assertion and dbs->acb is set to NULL again at line 121. Thus when
>>> we run assert at line 181 it will fail.
>>>
>>>   softmmu/dma-helpers.c:181: dma_blk_cb: Assertion `dbs->acb' failed.
>>>
>>> Reported-by: Francisco Londono <f.londono@samsung.com>
>>> Signed-off-by: Tong Zhang <t.zhang2@samsung.com>
>>> ---
>>>  softmmu/dma-helpers.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
>>> index 7820fec54c..cb81017928 100644
>>> --- a/softmmu/dma-helpers.c
>>> +++ b/softmmu/dma-helpers.c
>>> @@ -177,8 +177,8 @@ static void dma_blk_cb(void *opaque, int ret)
>>>      aio_context_acquire(dbs->ctx);
>>>      dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
>>>                              dma_blk_cb, dbs, dbs->io_func_opaque);
>>> -    aio_context_release(dbs->ctx);
>>>      assert(dbs->acb);
>>> +    aio_context_release(dbs->ctx);
>>>  }
>>>
>>>  static void dma_aio_cancel(BlockAIOCB *acb)
>>
>> I'm fairly new to that code, but I wonder what prevents dma_blk_cb() to
>> run after you reshuffled the code?
>>
> 
> IMO if the assert is to test whether io_func returns a non-NULL value
> shouldn't it be immediately after calling io_func.
> Also... as suggested by commit 6bee44ea346aed24e12d525daf10542d695508db
>   >     dma: the passed io_func does not return NULL

Yes, but I just don't see how it would fix the assertion you document in
the patch description. The locking change to fix the assertion doesn't
make any sense to me, and most probably I am missing something important :)

-- 
Thanks,

David / dhildenb