From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8jcN-0003Eh-PF for qemu-devel@nongnu.org; Tue, 15 Dec 2015 02:01:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8jcK-0000zh-I2 for qemu-devel@nongnu.org; Tue, 15 Dec 2015 02:01:51 -0500 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]:38644) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8jcK-0000zd-7C for qemu-devel@nongnu.org; Tue, 15 Dec 2015 02:01:48 -0500 Received: by mail-wm0-x236.google.com with SMTP id p66so11057779wmp.1 for ; Mon, 14 Dec 2015 23:01:47 -0800 (PST) Content-Type: multipart/alternative; boundary="Apple-Mail=_929B5AFD-E09A-4281-A285-9599213CD63F" Mime-Version: 1.0 (Mac OS X Mail 9.1 \(3096.5\)) From: Dmitry Fleytman In-Reply-To: Date: Tue, 15 Dec 2015 09:01:46 +0200 Message-Id: <71F8314F-F19C-47D0-8038-39D664E8DC79@daynix.com> References: <66A887B2-7CFF-45F9-AD7F-1381F8B1F318@daynix.com> <566105A2.6040508@redhat.com> <1C7745A5-54CD-439B-8A4B-442D97C58823@daynix.com> Subject: Re: [Qemu-devel] net: vmxnet3: memory leakage issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qinghao Tang , Jason Wang , qemu-devel@nongnu.org --Apple-Mail=_929B5AFD-E09A-4281-A285-9599213CD63F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hello Prasad, Looks good. Reviewed-by: Dmitry Fleytman Regards, Dmitry > On 15 Dec 2015, at 08:57 AM, P J P wrote: >=20 > Hello Dmitry, >=20 > +-- On Mon, 14 Dec 2015, Dmitry Fleytman wrote --+ > | The patch looks basically good. > | The only issue I can think of is that now vmxnet_tx_pkt_uninit and=20= > | vmxnet_rx_pkt_uninit may be called a few times in a row. For example = guest=20 > | may quiesce device and then shutdown. In this case = vmxnet_tx_pkt_uninit may=20 > | try to free the same memory twice. Could you take a look at this = please? >=20 > Yes, I've added a check in vmxnet3_deactivate_device() to avoid = double free.=20 > Please see below an updated patch. >=20 > =3D=3D=3D > =46rom 3ef66b01874fcc2fe3bfc73d2b61ee3a5b29fdb6 Mon Sep 17 00:00:00 = 2001 > From: Prasad J Pandit > Date: Tue, 15 Dec 2015 12:17:28 +0530 > Subject: [PATCH] net: vmxnet3: avoid memory leakage in activate_device >=20 > 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. >=20 > Reported-by: Qinghao Tang > Signed-off-by: Prasad J Pandit > --- > hw/net/vmxnet3.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) >=20 > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 37373e5..2b4aad7 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -1194,8 +1194,13 @@ static void vmxnet3_reset_mac(VMXNET3State *s) >=20 > static void vmxnet3_deactivate_device(VMXNET3State *s) > { > - VMW_CBPRN("Deactivating vmxnet3..."); > - s->device_active =3D false; > + if (s->device_active) { > + 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 =3D false; > + } > } >=20 > static void vmxnet3_reset(VMXNET3State *s) > @@ -1204,7 +1209,6 @@ static void vmxnet3_reset(VMXNET3State *s) >=20 > vmxnet3_deactivate_device(s); > vmxnet3_reset_interrupt_states(s); > - vmxnet_tx_pkt_reset(s->tx_pkt); > s->drv_shmem =3D 0; > s->tx_sop =3D true; > s->skip_current_tx_pkt =3D false; > @@ -1431,6 +1435,12 @@ static void = vmxnet3_activate_device(VMXNET3State *s) > return; > } >=20 > + /* 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 +1637,7 @@ static void vmxnet3_handle_command(VMXNET3State = *s, uint64_t cmd) > break; >=20 > 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; >=20 > @@ -1741,7 +1751,7 @@ vmxnet3_io_bar1_write(void *opaque, > * shared address only after we get the high part > */ > if (val =3D=3D 0) { > - s->device_active =3D false; > + vmxnet3_deactivate_device(s); > } > s->temp_shared_guest_driver_memory =3D val; > s->drv_shmem =3D 0; > @@ -2021,9 +2031,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); > } >=20 > --=20 > 2.4.3 > =3D=3D=3D >=20 >=20 > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F --Apple-Mail=_929B5AFD-E09A-4281-A285-9599213CD63F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii Hello Prasad,

Looks good.

Reviewed-by: = Dmitry Fleytman <dmitry@daynix.com>
=

Regards,
Dmitry

On 15 Dec 2015, at 08:57 AM, P J P <ppandit@redhat.com> = wrote:

  Hello Dmitry,

+-- = On Mon, 14 Dec 2015, Dmitry Fleytman wrote --+
| The patch = looks basically good.
| The only issue I can think of is = that now vmxnet_tx_pkt_uninit and
| vmxnet_rx_pkt_uninit = may be called a few times in a row. For example guest
| = may quiesce device and then shutdown. In this case vmxnet_tx_pkt_uninit = may
| try to free the same memory twice. Could you take a = look at this please?

 Yes, I've added = a check in vmxnet3_deactivate_device() to avoid double free.
Please see below an updated patch.

=3D=3D=3D
=46rom = 3ef66b01874fcc2fe3bfc73d2b61ee3a5b29fdb6 Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Tue, 15 Dec = 2015 12:17:28 +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 <luodalongde@gmail.com>
Signed-off-by: = Prasad J Pandit <pjp@fedoraproject.org>
---
= hw/net/vmxnet3.c | 24 ++++++++++++++++--------
1 file = changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 37373e5..2b4aad7 100644
--- = a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ = -1194,8 +1194,13 @@ static void vmxnet3_reset_mac(VMXNET3State *s)

static void = vmxnet3_deactivate_device(VMXNET3State *s)
{
-    VMW_CBPRN("Deactivating vmxnet3...");
-    s->device_active =3D false;
+    if (s->device_active) {
+ =        VMW_CBPRN("Deactivating = vmxnet3...");
+ =        vmxnet_tx_pkt_reset(s->tx_pkt= );
+ =        vmxnet_tx_pkt_uninit(s->tx_pk= t);
+ =        vmxnet_rx_pkt_uninit(s->rx_pk= t);
+ =        s->device_active =3D = false;
+    }
}

static void vmxnet3_reset(VMXNET3State *s)
@@ -1204,7 +1209,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 =3D 0;
=     s->tx_sop =3D true;
=     s->skip_current_tx_pkt =3D false;
@@ -1431,6 +1435,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 +1637,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 +1751,7 @@ vmxnet3_io_bar1_write(void *opaque,
         * = shared address only after we get the high part
=          */
=         if (val =3D=3D 0) {
- =            s->de= vice_active =3D false;
+ =            vmxnet3_= deactivate_device(s);
=         }
=         s->temp_shared_guest_dr= iver_memory =3D val;
=         s->drv_shmem =3D = 0;
@@ -2021,9 +2031,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
=3D=3D=3D<= br class=3D"">

Thank you.
--Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

= --Apple-Mail=_929B5AFD-E09A-4281-A285-9599213CD63F--