From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44866) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8RmA-0001Lc-U2 for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:58:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8Rm6-0005kl-RQ for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:58:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42562) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8Rm6-0005ke-Ji for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:58:42 -0500 Date: Mon, 14 Dec 2015 17:28:35 +0530 (IST) From: P J P In-Reply-To: <1C7745A5-54CD-439B-8A4B-442D97C58823@daynix.com> Message-ID: References: <66A887B2-7CFF-45F9-AD7F-1381F8B1F318@daynix.com> <566105A2.6040508@redhat.com> <1C7745A5-54CD-439B-8A4B-442D97C58823@daynix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Subject: Re: [Qemu-devel] net: vmxnet3: memory leakage issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Fleytman Cc: Qinghao Tang , Jason Wang , qemu-devel@nongnu.org Hello Dmitry, Jason +-- On Sun, 13 Dec 2015, Dmitry Fleytman wrote --+ | According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip | paused/active states. It always disables device, see vmxnet3_resume() for | | ): | | Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and then | performs activate sequence to launch the device. Yes, I did look through it. But it wasn't clear how it does flow control. As it resets the device on pause and loses any outstanding data. Whereas in the vmxnet3 emulator, upon deactivation it merely sets the 's->active_device' flag to be false, and the same is checked before receiving new packets. Do either of the vmxnet3 implementations perform flow control?(to avoid congestion) | So the correct fix should: | | 1. On device activation: check if device is active - do nothing | 2. In all places that set device_active to false, i.e. device quiesce, reset and VMXNET3_REG_DSAL set to zero: deallocate tx/rx packets as done in vmxnet3_net_uninit(): | | net_tx_pkt_reset(s->tx_pkt); | net_tx_pkt_uninit(s->tx_pkt); | net_rx_pkt_uninit(s->rx_pkt); | | It could be a good idea to extend vmxnet3_deactivate_device() with those | lines and call it from every place that sets device_active to false or frees | TX/RX packets. Right. Please see below a new tested patch which does this and fixes the host memory leakage issue. Does it look good? === >>From d4b277788d518e915cc6c20488d587cb5716e96a Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Mon, 14 Dec 2015 16:56:52 +0530 Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device Vmxnet3 device emulator does not check if the device is active before activating it, also it did not free the transmit & receive buffers while deactivating the device, thus resulting in memory leakage on the host. This patch fixes both these issues to avoid host memory leakage. Reported-by: Qinghao Tang Signed-off-by: Prasad J Pandit --- hw/net/vmxnet3.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 37373e5..3936f12 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -1195,6 +1195,9 @@ static void vmxnet3_reset_mac(VMXNET3State *s) static void vmxnet3_deactivate_device(VMXNET3State *s) { VMW_CBPRN("Deactivating vmxnet3..."); + vmxnet_tx_pkt_reset(s->tx_pkt); + vmxnet_tx_pkt_uninit(s->tx_pkt); + vmxnet_rx_pkt_uninit(s->rx_pkt); s->device_active = false; } @@ -1204,7 +1207,6 @@ static void vmxnet3_reset(VMXNET3State *s) vmxnet3_deactivate_device(s); vmxnet3_reset_interrupt_states(s); - vmxnet_tx_pkt_reset(s->tx_pkt); s->drv_shmem = 0; s->tx_sop = true; s->skip_current_tx_pkt = false; @@ -1431,6 +1433,12 @@ static void vmxnet3_activate_device(VMXNET3State *s) return; } + /* Verify if device is active */ + if (s->device_active) { + VMW_CFPRN("Vmxnet3 device is active"); + return; + } + vmxnet3_adjust_by_guest_type(s); vmxnet3_update_features(s); vmxnet3_update_pm_state(s); @@ -1627,7 +1635,7 @@ static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd) break; case VMXNET3_CMD_QUIESCE_DEV: - VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device"); + VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - deactivate the device"); vmxnet3_deactivate_device(s); break; @@ -1741,7 +1749,7 @@ vmxnet3_io_bar1_write(void *opaque, * shared address only after we get the high part */ if (val == 0) { - s->device_active = false; + vmxnet3_deactivate_device(s); } s->temp_shared_guest_driver_memory = val; s->drv_shmem = 0; @@ -2021,9 +2029,7 @@ static bool vmxnet3_peer_has_vnet_hdr(VMXNET3State *s) static void vmxnet3_net_uninit(VMXNET3State *s) { g_free(s->mcast_list); - vmxnet_tx_pkt_reset(s->tx_pkt); - vmxnet_tx_pkt_uninit(s->tx_pkt); - vmxnet_rx_pkt_uninit(s->rx_pkt); + vmxnet3_deactivate_device(s); qemu_del_nic(s->nic); } -- 2.4.3 === Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F