From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56494) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8WuR-00086m-Ac for qemu-devel@nongnu.org; Mon, 14 Dec 2015 12:27:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8WuO-000290-0C for qemu-devel@nongnu.org; Mon, 14 Dec 2015 12:27:39 -0500 Received: from mail-wm0-x232.google.com ([2a00:1450:400c:c09::232]:35287) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8WuN-00028p-NP for qemu-devel@nongnu.org; Mon, 14 Dec 2015 12:27:35 -0500 Received: by mail-wm0-x232.google.com with SMTP id p66so54674925wmp.0 for ; Mon, 14 Dec 2015 09:27:35 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 9.1 \(3096.5\)) From: Dmitry Fleytman In-Reply-To: Date: Mon, 14 Dec 2015 19:27:21 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: 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 Hello Prasad, > On 14 Dec 2015, at 13:58 PM, P J P wrote: >=20 > Hello Dmitry, Jason >=20 > +-- On Sun, 13 Dec 2015, Dmitry Fleytman wrote --+ > | According to Linux driver code VMXNET3_CMD_QUIESCE_DEV does not flip=20= > | paused/active states. It always disables device, see = vmxnet3_resume() for=20 > | > | = ): > |=20 > | Driver issues VMXNET3_CMD_QUIESCE_DEV to clear the device state and = then=20 > | performs activate sequence to launch the device. >=20 > Yes, I did look through it. But it wasn't clear how it does flow = control. As=20 > it resets the device on pause and loses any outstanding data. Whereas = in the=20 > vmxnet3 emulator, upon deactivation it merely sets the = 's->active_device' flag=20 > to be false, and the same is checked before receiving new packets. Do = either=20 > of the vmxnet3 implementations perform flow control?(to avoid = congestion) It=E2=80=99s hard to say as spec is not available. >=20 > | So the correct fix should: > |=20 > | 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(): > |=20 > | net_tx_pkt_reset(s->tx_pkt); > | net_tx_pkt_uninit(s->tx_pkt); > | net_rx_pkt_uninit(s->rx_pkt); > |=20 > | It could be a good idea to extend vmxnet3_deactivate_device() with = those=20 > | lines and call it from every place that sets device_active to false = or frees=20 > | TX/RX packets. >=20 > Right. Please see below a new tested patch which does this and fixes = the=20 > host memory leakage issue. Does it look good? >=20 > =3D=3D=3D > =46rom 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 >=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 | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) >=20 > 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 =3D false; > } >=20 > @@ -1204,7 +1207,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 +1433,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 +1635,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 +1749,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 +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); > } 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? Thanks for working on this, Dmitry >=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