Re: [Qemu-devel] net: vmxnet3: memory leakage issue

[P J P]

   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 <address@hidden>
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 <address@hidden>
Signed-off-by: Prasad J Pandit <address@hidden>
---
 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