From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33411) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6gf8-00065X-LD for qemu-devel@nongnu.org; Wed, 09 Dec 2015 10:28:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6gf3-0008GY-Mi for qemu-devel@nongnu.org; Wed, 09 Dec 2015 10:28:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33860) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6gf3-0008GC-F8 for qemu-devel@nongnu.org; Wed, 09 Dec 2015 10:28:09 -0500 Date: Wed, 9 Dec 2015 20:58:01 +0530 (IST) From: P J P In-Reply-To: Message-ID: References: <66A887B2-7CFF-45F9-AD7F-1381F8B1F318@daynix.com> <566105A2.6040508@redhat.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: qemu-devel@nongnu.org Cc: Dmitry Fleytman , Qinghao Tang , Jason Wang Hello Jason, Dmitry, +-- On Tue, 8 Dec 2015, P J P wrote --+ | |1) VMXNET3_CMD_QUIESCE_DEV | | IIUC, it is used to pause the device when the receiver end is unable to | keee-up with the incoming flow. After a brief period, the operation could be | resumed again. | | |2) VMXNET3_REG_DSAL | | Shared memory between a driver and the device appears to be set in two | steps. Firs low address, followed by the high address(VMXNET3_REG_DSAH). I | guess 's->device_active' needs to be enabled again while setting the higher | part of the address. Please see below another (tested)patch, it fixes the VMXNET3_CMD_QUIESCE_DEV case above. It's not clear if the device would be initialised and active while setting shared memory via VMXNET3_REG_DSAL/DSAH. === >>From 81c4ecb67635435f01397dc21210497e6420bdca Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Wed, 9 Dec 2015 20:45:25 +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, resulting in memory leakage on the host. This memory leakage could also occur, if the device was paused during flow control and activated thereafter. Introduced a 'Pause' state for the vmxnet3 device to differentiate it from the earlier active and inactive states. One need not 'activate' the device to resume operation after pause. This patch adds a check to verify these device states and avoid memory leakage during activating the device. Reported-by: Qinghao Tang Signed-off-by: Prasad J Pandit --- hw/net/vmxnet3.c | 43 +++++++++++++++++++++++++++++++++++-------- hw/net/vmxnet3.h | 6 ++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 37373e5..be2c9e2 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -1195,7 +1195,19 @@ static void vmxnet3_reset_mac(VMXNET3State *s) static void vmxnet3_deactivate_device(VMXNET3State *s) { VMW_CBPRN("Deactivating vmxnet3..."); - s->device_active = false; + s->device_active = VMXNET3_DEV_DEACTIVE; +} + +static void vmxnet3_pause_device(VMXNET3State *s) +{ + VMW_CBPRN("Pausing vmxnet3..."); + s->device_active = VMXNET3_DEV_PAUSE; +} + +static void vmxnet3_resume_device(VMXNET3State *s) +{ + VMW_CBPRN("Resuming vmxnet3..."); + s->device_active = VMXNET3_DEV_ACTIVE; } static void vmxnet3_reset(VMXNET3State *s) @@ -1431,6 +1443,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); @@ -1566,7 +1584,7 @@ static void vmxnet3_activate_device(VMXNET3State *s) vmxnet3_reset_mac(s); - s->device_active = true; + s->device_active = VMXNET3_DEV_ACTIVE; } static void vmxnet3_handle_command(VMXNET3State *s, uint64_t cmd) @@ -1627,8 +1645,13 @@ 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"); - vmxnet3_deactivate_device(s); + if (s->device_active & VMXNET3_DEV_ACTIVE) { + VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - pause the device"); + vmxnet3_pause_device(s); + } else if (s->device_active & VMXNET3_DEV_PAUSE) { + VMW_CBPRN("Set: VMXNET3_CMD_QUIESCE_DEV - resume the device"); + vmxnet3_resume_device(s); + } break; case VMXNET3_CMD_GET_CONF_INTR: @@ -1652,12 +1675,16 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State *s) switch (s->last_command) { case VMXNET3_CMD_ACTIVATE_DEV: - ret = (s->device_active) ? 0 : -1; + ret = (s->device_active & VMXNET3_DEV_ACTIVE) ? 0 : -1; VMW_CFPRN("Device active: %" PRIx64, ret); break; - case VMXNET3_CMD_RESET_DEV: case VMXNET3_CMD_QUIESCE_DEV: + ret = (s->device_active & VMXNET3_DEV_PAUSE) ? 0 : -1; + VMW_CFPRN("Device pause: %" PRIx64, ret); + break; + + case VMXNET3_CMD_RESET_DEV: case VMXNET3_CMD_GET_QUEUE_STATUS: ret = 0; break; @@ -1741,7 +1768,7 @@ vmxnet3_io_bar1_write(void *opaque, * shared address only after we get the high part */ if (val == 0) { - s->device_active = false; + s->device_active = VMXNET3_DEV_DEACTIVE; } s->temp_shared_guest_driver_memory = val; s->drv_shmem = 0; @@ -1863,7 +1890,7 @@ static int vmxnet3_can_receive(NetClientState *nc) { VMXNET3State *s = qemu_get_nic_opaque(nc); - return s->device_active && + return (s->device_active & VMXNET3_DEV_ACTIVE) && VMXNET_FLAG_IS_SET(s->link_status_and_speed, VMXNET3_LINK_STATUS_UP); } diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h index f7006af..71a7861 100644 --- a/hw/net/vmxnet3.h +++ b/hw/net/vmxnet3.h @@ -202,6 +202,12 @@ enum { VMXNET3_CMD_GET_ADAPTIVE_RING_INFO /* 0xF00D0009 */ }; +enum { + VMXNET3_DEV_DEACTIVE = 0x00, /* device deactive */ + VMXNET3_DEV_ACTIVE = 0x01, /* device active */ + VMXNET3_DEV_PAUSE = 0x02 /* device pause */ +}; + /* Adaptive Ring Info Flags */ #define VMXNET3_DISABLE_ADAPTIVE_RING 1 -- 2.4.3 === Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F