Skip to content

Commit

Permalink
ahci: fix sglist leak on retry
Browse files Browse the repository at this point in the history
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
    open-power-host-os#4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844
    open-power-host-os#5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333
    open-power-host-os#6 0x556a187b650b in ide_start_dma hw/ide/core.c:921
    open-power-host-os#7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911
    open-power-host-os#8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486
    open-power-host-os#9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027
    open-power-host-os#10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204
    open-power-host-os#11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254
    open-power-host-os#12 0x556a187d168a in check_cmd hw/ide/ahci.c:510
    open-power-host-os#13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314
    open-power-host-os#14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435
    open-power-host-os#15 0x556a1831d959 in memory_region_write_accessor /home/elmarco/src/qemu/memory.c:525
    open-power-host-os#16 0x556a1831dc35 in access_with_adjusted_size /home/elmarco/src/qemu/memory.c:591
    open-power-host-os#17 0x556a18323ce3 in memory_region_dispatch_write /home/elmarco/src/qemu/memory.c:1262
    open-power-host-os#18 0x556a1828cf67 in address_space_write_continue /home/elmarco/src/qemu/exec.c:2578
    open-power-host-os#19 0x556a1828d20b in address_space_write /home/elmarco/src/qemu/exec.c:2635
    open-power-host-os#20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737
    open-power-host-os#21 0x556a1828daf7 in cpu_physical_memory_rw /home/elmarco/src/qemu/exec.c:2746
    open-power-host-os#22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qemu/include/exec/cpu-common.h:72
    open-power-host-os#23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qtest.c:382
    open-power-host-os#24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtest.c:573
    open-power-host-os#25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585
    open-power-host-os#26 0x556a18598b85 in qemu_chr_be_write_impl /home/elmarco/src/qemu/qemu-char.c:387
    open-power-host-os#27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-char.c:399
    open-power-host-os#28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902
    open-power-host-os#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é Lureau <marcandre.lureau@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
  • Loading branch information
elmarco committed Aug 7, 2016
1 parent 9ef6172 commit 5839df7
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
3 changes: 1 addition & 2 deletions hw/ide/ahci.c
Expand Up @@ -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;
}

Expand Down Expand Up @@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *ncq_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);
}
}
Expand Down Expand Up @@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int port, 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;
Expand Down
1 change: 1 addition & 0 deletions hw/ide/core.c
Expand Up @@ -824,6 +824,7 @@ static void ide_dma_cb(void *opaque, int ret)
if (ret < 0) {
if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
s->bus->dma->aiocb = NULL;
dma_buf_commit(s, 0);
return;
}
}
Expand Down

0 comments on commit 5839df7

Please sign in to comment.