On 9/22/20 12:37 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <phi...@redhat.com> 于2020年9月22日周二 下午4:19写道：
>>
>> On 9/22/20 3:34 AM, Alexander Bulekov wrote:
>>> On 200815 0020, Li Qiang wrote:
>>>> In 'map_page' we need to check the return value of
>>>> 'dma_memory_map' to ensure the we actully maped something.
>>>> Otherwise, we will hit an assert in 'address_space_unmap'.
>>>> This is because we can't find the MR with the NULL buffer.
>>>> This is the LP#1884693:
>>>>
>>>> -->https://bugs.launchpad.net/qemu/+bug/1884693
>>>>
>>>> Reported-by: Alexander Bulekov <alx...@bu.edu>
>>>> Signed-off-by: Li Qiang <liq...@163.com>
>>>
>>> I'm not very familiar with the IDE code, but this seems like a simple
>>> null-ptr check, and Li has not received a response in over a month.
>>
>> Yeah well it is not an easy bug... I spent few hours but at some
>> point it became too AHCI specific. I wanted to understand the bug
>> to answer the "Why do we get there?" "Can we get there with real
>> hardware?" questions, to be able to discern if this patch is OK,
>> or if it is hiding bugs and what we really use here is an assert().
>
> Hi Philippe,
> I think you have complicated this issue. The root cause is that
> 'dma_memory_map' maybe fail.
> The gpa is from guest and can be any value so this is expected.
> It can return NULL pointer (no map) or it can be do partially
> mapped(len < wanted).
> Though in most situation the map result is 'ret == NULL and len <
> wanted'. It may also has '
> ret != NULL and len < wanted' I think.
Then this form is easier to review to my taste:
-- >8 --
@@ -250,7 +250,7 @@ static void map_page(AddressSpace *as, uint8_t
**ptr, uint64_t addr,
}
*ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
- if (len < wanted) {
+ if (*ptr && len < wanted) {
dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
*ptr = NULL;
}
---
>
> The assert is come from that we pass NULL to 'dma_memory_unmap'.
>
> So the standard usage of 'dma_memory_map' I think is first check if
> the return value to ensure it is not NULL.
> Then check whether it mapped the len as the caller expected.
>
> There are several places in the code base that doesn't following this
> usage which I think it is wrong.
>
> Thanks,
> Li Qiang
>
>>
>>>
>>> Reviewed-by: Alexander Bulekov <alx...@bu.edu>
>>>
>>>> ---
>>>> hw/ide/ahci.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>> index 009120f88b..63e9fccdbe 100644
>>>> --- a/hw/ide/ahci.c
>>>> +++ b/hw/ide/ahci.c
>>>> @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
>>>> uint64_t addr,
>>>> }
>>>>
>>>> *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
>>>> +
>>>> + if (!*ptr) {
>>>> + return;
>>>> + }
>>>> +
>>>> if (len < wanted) {
>>>> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>>>> *ptr = NULL;
>>>> --
>>>> 2.17.1
>>>>
>>>
>>
>
