qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 36/37] ahci: fix sglist leak on retry


From: John Snow
Subject: Re: [Qemu-devel] [PATCH 36/37] ahci: fix sglist leak on retry
Date: Tue, 19 Jul 2016 16:45:17 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1


On 07/19/2016 04:54 AM, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> ahci-test /x86_64/ahci/io/dma/lba28/retry triggers the following leak:
> 
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x7fc4b2a25e20 in malloc (/lib64/libasan.so.3+0xc6e20)
>     #1 0x7fc4993bce58 in g_malloc (/lib64/libglib-2.0.so.0+0x4ee58)
>     #2 0x556a187d4b34 in ahci_populate_sglist hw/ide/ahci.c:896
>     #3 0x556a187d8237 in ahci_dma_prepare_buf hw/ide/ahci.c:1367
>     #4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844
>     #5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333
>     #6 0x556a187b650b in ide_start_dma hw/ide/core.c:921
>     #7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911
>     #8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486
>     #9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027
>     #10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204
>     #11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254
>     #12 0x556a187d168a in check_cmd hw/ide/ahci.c:510
>     #13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314
>     #14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435
>     #15 0x556a1831d959 in memory_region_write_accessor 
> /home/elmarco/src/qemu/memory.c:525
>     #16 0x556a1831dc35 in access_with_adjusted_size 
> /home/elmarco/src/qemu/memory.c:591
>     #17 0x556a18323ce3 in memory_region_dispatch_write 
> /home/elmarco/src/qemu/memory.c:1262
>     #18 0x556a1828cf67 in address_space_write_continue 
> /home/elmarco/src/qemu/exec.c:2578
>     #19 0x556a1828d20b in address_space_write 
> /home/elmarco/src/qemu/exec.c:2635
>     #20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737
>     #21 0x556a1828daf7 in cpu_physical_memory_rw 
> /home/elmarco/src/qemu/exec.c:2746
>     #22 0x556a183068d3 in cpu_physical_memory_write 
> /home/elmarco/src/qemu/include/exec/cpu-common.h:72
>     #23 0x556a18308194 in qtest_process_command 
> /home/elmarco/src/qemu/qtest.c:382
>     #24 0x556a18309999 in qtest_process_inbuf 
> /home/elmarco/src/qemu/qtest.c:573
>     #25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585
>     #26 0x556a18598b85 in qemu_chr_be_write_impl 
> /home/elmarco/src/qemu/qemu-char.c:387
>     #27 0x556a18598c52 in qemu_chr_be_write 
> /home/elmarco/src/qemu/qemu-char.c:399
>     #28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902
>     #29 0x556a18cbaf52 in qio_channel_fd_source_dispatch io/channel-watch.c:84
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/ide/ahci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 6defeed..0ea5805 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -919,6 +919,7 @@ static void ncq_err(NCQTransferState *ncq_tfs)
>      ide_state->error = ABRT_ERR;
>      ide_state->status = READY_STAT | ERR_STAT;
>      ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
> +    qemu_sglist_destroy(&ncq_tfs->sglist);
>      ncq_tfs->used = 0;
>  }
>  

I wish I could remember why I didn't want to put the sglist_destroy into
ncq_err ... Well, probably not important.

Everywhere else ncq_err is used, it is accompanied by a list cleanup
except for ncq_cb, which is the case you are fixing here.

Move the sglist destruction inside of ncq_err and then delete it from
the other two locations to keep it tidy.

> @@ -1330,6 +1331,7 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>      DPRINTF(ad->port_no, "\n");
>      s->io_buffer_offset = 0;
> +    qemu_sglist_destroy(&s->sg);

Hm, I'd rather the list destruction go in ide_dma_cb when it has become
clear we are going to be halting instead of in ahci_start_dma.

Maybe dma_buf_commit in ide_dma_cb after the early return?

(Though, this is also a little wonky because this routine does more than
clear the list, but it is at the moment the centralized "we're done with
the sglist" function and none of the other side effects that occur in
dma_buf_commit will interfere with the reset that occurs from
ide_restart_bh, I think.)

>      dma_cb(s, 0);
>  }
>  
> 

Thanks!
--js



reply via email to

[Prev in Thread] Current Thread [Next in Thread]