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

[P J P]

  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 
|
| 
<https://lxr.missinglinkelectronics.com/linux/drivers/net/vmxnet3/vmxnet3_drv.c#L3423>):
| 
| 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 <address@hidden>
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 <address@hidden>
Signed-off-by: Prasad J Pandit <address@hidden>
---
 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