From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVJui-0007T3-Ig for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:46:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVJue-0000Uy-Aw for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:46:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39842) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVJue-0000Ut-2n for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:46:20 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9CEF7C0624A1 for ; Thu, 4 Aug 2016 14:46:19 +0000 (UTC) From: Markus Armbruster References: <20160803145541.15355-1-marcandre.lureau@redhat.com> <20160803145541.15355-36-marcandre.lureau@redhat.com> Date: Thu, 04 Aug 2016 16:46:17 +0200 In-Reply-To: <20160803145541.15355-36-marcandre.lureau@redhat.com> (marcandre lureau's message of "Wed, 3 Aug 2016 18:55:40 +0400") Message-ID: <87invgd22u.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-2.7 v3 35/36] ahci: fix sglist leak on retry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, John Snow John, can you review this one? marcandre.lureau@redhat.com writes: > From: Marc-Andr=C3=A9 Lureau > > 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/qem= u/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/e= xec.c:2746 > #22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qem= u/include/exec/cpu-common.h:72 > #23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qt= est.c:382 > #24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtes= t.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/q= emu-char.c:387 > #27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-c= har.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 > > Follow John Snow recommendation: > 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. > > Call 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 > > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > hw/ide/ahci.c | 3 +-- > hw/ide/core.c | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 6defeed..f3438ad 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 =3D ABRT_ERR; > ide_state->status =3D READY_STAT | ERR_STAT; > ncq_tfs->drive->port_regs.scr_err |=3D (1 << ncq_tfs->tag); > + qemu_sglist_destroy(&ncq_tfs->sglist); > ncq_tfs->used =3D 0; > } >=20=20 > @@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *n= cq_tfs) > default: > DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\= n", > ncq_tfs->cmd); > - qemu_sglist_destroy(&ncq_tfs->sglist); > ncq_err(ncq_tfs); > } > } > @@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int p= ort, uint8_t *cmd_fis, > error_report("ahci: PRDT length for NCQ command (0x%zx) " > "is smaller than the requested size (0x%zx)", > ncq_tfs->sglist.size, size); > - qemu_sglist_destroy(&ncq_tfs->sglist); > ncq_err(ncq_tfs); > ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW); > return; > diff --git a/hw/ide/core.c b/hw/ide/core.c > index f9c8162..82d7f2a 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -822,6 +822,7 @@ static void ide_dma_cb(void *opaque, int ret) > return; > } > if (ret < 0) { > + dma_buf_commit(s, 0); > if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd= ))) { > s->bus->dma->aiocb =3D NULL; > return;