diff mbox series

xhci: Fix memory leak in xhci_kick_epctx when poweroff GuestOS

Message ID 20200110105855.81144-1-kuhn.chenqun@huawei.com
State New
Headers show
Series xhci: Fix memory leak in xhci_kick_epctx when poweroff GuestOS | expand

Commit Message

Chen Qun Jan. 10, 2020, 10:58 a.m. UTC 
From: Chen Qun <kuhn.chenqun@huawei.com>

start vm with libvirt, when GuestOS running, enter poweroff command using
the xhci keyboard, then ASAN shows memory leak stack：

Direct leak of 80 byte(s) in 5 object(s) allocated from:
    #0 0xfffd1e6431cb in __interceptor_malloc (/lib64/libasan.so.4+0xd31cb)
    #1 0xfffd1e107163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
    #2 0xaaad39051367 in qemu_sglist_init /qemu/dma-helpers.c:43
    #3 0xaaad3947c407 in pci_dma_sglist_init /qemu/include/hw/pci/pci.h:842
    #4 0xaaad3947c407 in xhci_xfer_create_sgl /qemu/hw/usb/hcd-xhci.c:1446
    #5 0xaaad3947c407 in xhci_setup_packet /qemu/hw/usb/hcd-xhci.c:1618
    #6 0xaaad3948625f in xhci_submit /qemu/hw/usb/hcd-xhci.c:1827
    #7 0xaaad3948625f in xhci_fire_transfer /qemu/hw/usb/hcd-xhci.c:1839
    #8 0xaaad3948625f in xhci_kick_epctx /qemu/hw/usb/hcd-xhci.c:1991
    #9 0xaaad3948f537 in xhci_doorbell_write /qemu/hw/usb/hcd-xhci.c:3158
    #10 0xaaad38bcbfc7 in memory_region_write_accessor /qemu/memory.c:483
    #11 0xaaad38bc654f in access_with_adjusted_size /qemu/memory.c:544
    #12 0xaaad38bd1877 in memory_region_dispatch_write /qemu/memory.c:1482
    #13 0xaaad38b1c77f in flatview_write_continue /qemu/exec.c:3167
    #14 0xaaad38b1ca83 in flatview_write /qemu/exec.c:3207
    #15 0xaaad38b268db in address_space_write /qemu/exec.c:3297
    #16 0xaaad38bf909b in kvm_cpu_exec /qemu/accel/kvm/kvm-all.c:2383
    #17 0xaaad38bb063f in qemu_kvm_cpu_thread_fn /qemu/cpus.c:1246
    #18 0xaaad39821c93 in qemu_thread_start /qemu/util/qemu-thread-posix.c:519
    #19 0xfffd1c8378bb  (/lib64/libpthread.so.0+0x78bb)
    #20 0xfffd1c77616b  (/lib64/libc.so.6+0xd616b)

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
 hw/usb/hcd-xhci.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Philippe Mathieu-Daudé Jan. 10, 2020, 1:14 p.m. UTC | #1 
On 1/10/20 11:58 AM, kuhn.chenqun@huawei.com wrote:
> From: Chen Qun <kuhn.chenqun@huawei.com>
> 
> start vm with libvirt, when GuestOS running, enter poweroff command using
> the xhci keyboard, then ASAN shows memory leak stack：
> 
> Direct leak of 80 byte(s) in 5 object(s) allocated from:
>      #0 0xfffd1e6431cb in __interceptor_malloc (/lib64/libasan.so.4+0xd31cb)
>      #1 0xfffd1e107163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
>      #2 0xaaad39051367 in qemu_sglist_init /qemu/dma-helpers.c:43
>      #3 0xaaad3947c407 in pci_dma_sglist_init /qemu/include/hw/pci/pci.h:842
>      #4 0xaaad3947c407 in xhci_xfer_create_sgl /qemu/hw/usb/hcd-xhci.c:1446
>      #5 0xaaad3947c407 in xhci_setup_packet /qemu/hw/usb/hcd-xhci.c:1618
>      #6 0xaaad3948625f in xhci_submit /qemu/hw/usb/hcd-xhci.c:1827
>      #7 0xaaad3948625f in xhci_fire_transfer /qemu/hw/usb/hcd-xhci.c:1839
>      #8 0xaaad3948625f in xhci_kick_epctx /qemu/hw/usb/hcd-xhci.c:1991
>      #9 0xaaad3948f537 in xhci_doorbell_write /qemu/hw/usb/hcd-xhci.c:3158
>      #10 0xaaad38bcbfc7 in memory_region_write_accessor /qemu/memory.c:483
>      #11 0xaaad38bc654f in access_with_adjusted_size /qemu/memory.c:544
>      #12 0xaaad38bd1877 in memory_region_dispatch_write /qemu/memory.c:1482
>      #13 0xaaad38b1c77f in flatview_write_continue /qemu/exec.c:3167
>      #14 0xaaad38b1ca83 in flatview_write /qemu/exec.c:3207
>      #15 0xaaad38b268db in address_space_write /qemu/exec.c:3297
>      #16 0xaaad38bf909b in kvm_cpu_exec /qemu/accel/kvm/kvm-all.c:2383
>      #17 0xaaad38bb063f in qemu_kvm_cpu_thread_fn /qemu/cpus.c:1246
>      #18 0xaaad39821c93 in qemu_thread_start /qemu/util/qemu-thread-posix.c:519
>      #19 0xfffd1c8378bb  (/lib64/libpthread.so.0+0x78bb)
>      #20 0xfffd1c77616b  (/lib64/libc.so.6+0xd616b)
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
>   hw/usb/hcd-xhci.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 80988bb305..0d3d96d05a 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
>           if (xfer != NULL && xfer->running_retry) {
>               DPRINTF("xhci: xfer nacked, stopping schedule\n");
>               epctx->retry = xfer;
> +            xhci_xfer_unmap(xfer);

Shouldn't we use xhci_ep_free_xfer() instead?
Also, it would be cleaner if you set it to NULL.

>               break;
>           }
>           if (count++ > TRANSFER_LIMIT) {
>
Chen Qun Jan. 11, 2020, 2:15 a.m. UTC | #2 
>-----Original Message-----
>From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>Sent: Friday, January 10, 2020 9:14 PM
>To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>devel@nongnu.org; kraxel@redhat.com
>Cc: qemu-trivial@nongnu.org; Pannengyuan <pannengyuan@huawei.com>;
>Zhanghailiang <zhang.zhanghailiang@huawei.com>
>Subject: Re: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff
>GuestOS
>
>On 1/10/20 11:58 AM, kuhn.chenqun@huawei.com wrote:
>> From: Chen Qun <kuhn.chenqun@huawei.com>
>>
>> start vm with libvirt, when GuestOS running, enter poweroff command
>> using the xhci keyboard, then ASAN shows memory leak stack：
>>
>> Direct leak of 80 byte(s) in 5 object(s) allocated from:
>>      #0 0xfffd1e6431cb in __interceptor_malloc (/lib64/libasan.so.4+0xd31cb)
>>      #1 0xfffd1e107163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
>>      #2 0xaaad39051367 in qemu_sglist_init /qemu/dma-helpers.c:43
>>      #3 0xaaad3947c407 in pci_dma_sglist_init
>/qemu/include/hw/pci/pci.h:842
>>      #4 0xaaad3947c407 in xhci_xfer_create_sgl /qemu/hw/usb/hcd-
>xhci.c:1446
>>      #5 0xaaad3947c407 in xhci_setup_packet /qemu/hw/usb/hcd-xhci.c:1618
>>      #6 0xaaad3948625f in xhci_submit /qemu/hw/usb/hcd-xhci.c:1827
>>      #7 0xaaad3948625f in xhci_fire_transfer /qemu/hw/usb/hcd-xhci.c:1839
>>      #8 0xaaad3948625f in xhci_kick_epctx /qemu/hw/usb/hcd-xhci.c:1991
>>      #9 0xaaad3948f537 in xhci_doorbell_write /qemu/hw/usb/hcd-
>xhci.c:3158
>>      #10 0xaaad38bcbfc7 in memory_region_write_accessor
>/qemu/memory.c:483
>>      #11 0xaaad38bc654f in access_with_adjusted_size /qemu/memory.c:544
>>      #12 0xaaad38bd1877 in memory_region_dispatch_write
>/qemu/memory.c:1482
>>      #13 0xaaad38b1c77f in flatview_write_continue /qemu/exec.c:3167
>>      #14 0xaaad38b1ca83 in flatview_write /qemu/exec.c:3207
>>      #15 0xaaad38b268db in address_space_write /qemu/exec.c:3297
>>      #16 0xaaad38bf909b in kvm_cpu_exec /qemu/accel/kvm/kvm-all.c:2383
>>      #17 0xaaad38bb063f in qemu_kvm_cpu_thread_fn /qemu/cpus.c:1246
>>      #18 0xaaad39821c93 in qemu_thread_start /qemu/util/qemu-thread-
>posix.c:519
>>      #19 0xfffd1c8378bb  (/lib64/libpthread.so.0+0x78bb)
>>      #20 0xfffd1c77616b  (/lib64/libc.so.6+0xd616b)
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>>   hw/usb/hcd-xhci.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index
>> 80988bb305..0d3d96d05a 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx,
>unsigned int streamid)
>>           if (xfer != NULL && xfer->running_retry) {
>>               DPRINTF("xhci: xfer nacked, stopping schedule\n");
>>               epctx->retry = xfer;
>> +            xhci_xfer_unmap(xfer);
>
>Shouldn't we use xhci_ep_free_xfer() instead?
>Also, it would be cleaner if you set it to NULL.
>
Hi  Philippe,
Thanks for your reply!  
But,  It  is just a sglist leak.  xhci_xfer_unmap() can free and set  it to NULL.
The  xhci_ep_free_xfer() did't free a sglist.  

By the way, xfer should be use for epctx->retry, we can't free it.

Thanks.
>>               break;
>>           }
>>           if (count++ > TRANSFER_LIMIT) {
>>
Gerd Hoffmann Jan. 13, 2020, 7:48 a.m. UTC | #3 
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index 80988bb305..0d3d96d05a 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
> >           if (xfer != NULL && xfer->running_retry) {
> >               DPRINTF("xhci: xfer nacked, stopping schedule\n");
> >               epctx->retry = xfer;
> > +            xhci_xfer_unmap(xfer);
> 
> Shouldn't we use xhci_ep_free_xfer() instead?

No, xhci will try to run the transfer again later.

xhci will re-create the sgl then, so freeing the
sgl here is correct.  Patch added to usb queue.

thanks,
  Gerd
Chen Qun Jan. 13, 2020, 9:39 a.m. UTC | #4 
>-----Original Message-----
>From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>Sent: Monday, January 13, 2020 3:48 PM
>To: Philippe Mathieu-Daudé <philmd@redhat.com>
>Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>devel@nongnu.org; qemu-trivial@nongnu.org; Pannengyuan
><pannengyuan@huawei.com>; Zhanghailiang
><zhang.zhanghailiang@huawei.com>
>Subject: Re: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff
>GuestOS
>
>> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index
>> > 80988bb305..0d3d96d05a 100644
>> > --- a/hw/usb/hcd-xhci.c
>> > +++ b/hw/usb/hcd-xhci.c
>> > @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext
>*epctx, unsigned int streamid)
>> >           if (xfer != NULL && xfer->running_retry) {
>> >               DPRINTF("xhci: xfer nacked, stopping schedule\n");
>> >               epctx->retry = xfer;
>> > +            xhci_xfer_unmap(xfer);
>>
>> Shouldn't we use xhci_ep_free_xfer() instead?
>
>No, xhci will try to run the transfer again later.
>
>xhci will re-create the sgl then, so freeing the sgl here is correct.  Patch added
>to usb queue.

Hi  Gerd,

I test every keyboard input, it will leak once.   
I tested qemu-4.0.0  also had  this leak .

Maybe  we should cc to qemu-stable ?

Thanks.
>
>thanks,
>  Gerd
diff mbox series

Patch

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 80988bb305..0d3d96d05a 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2000,6 +2000,7 @@  static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
         if (xfer != NULL && xfer->running_retry) {
             DPRINTF("xhci: xfer nacked, stopping schedule\n");
             epctx->retry = xfer;
+            xhci_xfer_unmap(xfer);
             break;
         }
         if (count++ > TRANSFER_LIMIT) {