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

[Li Qiang <liq3ea@gmail.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年9月22日周二 下午6:46写道：
>
> 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;
>      }

Yes, in this case your patch is more clean and less code change.
But in generic case, my origin patch is more easy to understand and also
It is easy to do resource clean or trace (such as 'ahci_populate_sglist').

Anyway just let John do the choice.

Thanks,
Li Qiang

> ---
>
> >
> > 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
> >>>>
> >>>
> >>
> >
>