Re: [PATCH] hw: ide: check the pointer before do dma memory unmap

[Philippe Mathieu-Daudé]

On 9/22/20 12:37 PM, Li Qiang wrote:
> Philippe Mathieu-Daudé <philmd@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 <alxndr@bu.edu>
>>>> Signed-off-by: Li Qiang <liq3ea@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 <alxndr@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
>>>>
>>>
>>
>