hw: ide: check the pointer before do dma memory unmap

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

Posted by Li Qiang 3 years, 8 months ago

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

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

Posted by Alexander Bulekov 3 years, 7 months ago

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.

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
>

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

Posted by Philippe Mathieu-Daudé 3 years, 7 months ago

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().

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

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

Posted by Li Qiang 3 years, 7 months ago

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.

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

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

Posted by Philippe Mathieu-Daudé 3 years, 7 months ago

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

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

Posted by Li Qiang 3 years, 7 months ago

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

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

Posted by Li Qiang 3 years, 7 months ago

Ping.

Li Qiang <liq3ea@163.com> 于2020年8月15日周六 下午3:21写道：
>
> 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>
> ---
>  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
>

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

Posted by Li Qiang 3 years, 7 months ago

Ping!

Li Qiang <liq3ea@gmail.com> 于2020年9月1日周二 下午6:34写道：
>
> Ping.
>
> Li Qiang <liq3ea@163.com> 于2020年8月15日周六 下午3:21写道：
> >
> > 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>
> > ---
> >  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
> >

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

Posted by Li Qiang 3 years, 7 months ago

ping!!

Li Qiang <liq3ea@gmail.com> 于2020年9月7日周一 上午9:39写道：
>
> Ping!
>
> Li Qiang <liq3ea@gmail.com> 于2020年9月1日周二 下午6:34写道：
> >
> > Ping.
> >
> > Li Qiang <liq3ea@163.com> 于2020年8月15日周六 下午3:21写道：
> > >
> > > 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>
> > > ---
> > >  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
> > >

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

Posted by Li Qiang 3 years, 7 months ago

Ping!!

Li Qiang <liq3ea@gmail.com> 于2020年9月15日周二 下午9:38写道：
>
> ping!!
>
> Li Qiang <liq3ea@gmail.com> 于2020年9月7日周一 上午9:39写道：
> >
> > Ping!
> >
> > Li Qiang <liq3ea@gmail.com> 于2020年9月1日周二 下午6:34写道：
> > >
> > > Ping.
> > >
> > > Li Qiang <liq3ea@163.com> 于2020年8月15日周六 下午3:21写道：
> > > >
> > > > 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>
> > > > ---
> > > >  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
> > > >