From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 04B5E95B7 for ; Thu, 7 Jan 2016 07:50:14 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 06 Jan 2016 22:50:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,532,1444719600"; d="scan'208";a="24987365" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 06 Jan 2016 22:50:11 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 6 Jan 2016 22:50:11 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 6 Jan 2016 22:50:11 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.111]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.220]) with mapi id 14.03.0248.002; Thu, 7 Jan 2016 14:50:09 +0800 From: "Xie, Huawei" To: Yuanhan Liu , Rich Lane Thread-Topic: [PATCH] vhost: fix leak of fds and mmaps Thread-Index: AdFJF6YcQDq60L3DQYi/kaivI2An8A== Date: Thu, 7 Jan 2016 06:50:09 +0000 Message-ID: References: <1452032049-94324-1-git-send-email-rlane@bigswitch.com> <20160107023106.GI26062@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] vhost: fix leak of fds and mmaps X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jan 2016 06:50:15 -0000 On 1/7/2016 10:27 AM, Yuanhan Liu wrote:=0A= > On Tue, Jan 05, 2016 at 02:14:09PM -0800, Rich Lane wrote:=0A= >> The common vhost code only supported a single mmap per device. vhost-use= r=0A= >> worked around this by saving the address/length/fd of each mmap after th= e end=0A= >> of the rte_virtio_memory struct. This only works if the vhost-user code = frees=0A= >> dev->mem, since the common code is unaware of the extra info. The=0A= >> VHOST_USER_RESET_OWNER message is one situation where the common code fr= ees=0A= >> dev->mem and leaks the fds and mappings. This happens every time I shut = down a=0A= >> VM.=0A= > That is a good catch! But I don't think it needs the complexity your=0A= > patch has to fix it. Besides that, your patch has ABI change, which=0A= > is not acceptable at this stage.=0A= >=0A= > Back to the issue, the thing is that, mmap/unmap is vhost-user/vhost-cuse= =0A= > specific, thus we'd better to handle them inside vhost-user/vhost-cuse=0A= > differently, but not in the common path, virtio-net.c: let the common=0A= > path handle common things only. That would make the logic clear, and=0A= > hence less error prone.=0A= >=0A= > Note that we have already handled the mmap inside vhost-user/vhost-cuse,= =0A= > thinking of that way, there is no reason to handle unmap at virtio-net.c:= =0A= > it should be a historic issue while we added vhost-cuse first, which will= =0A= > not be an issue until we added vhost-user.=0A= =0A= Agree. Let vhost-user part handle its specific cleanup and let the=0A= virtio-net.c handle the common logic.=0A= =0A= >=0A= > Another note is that you should not name an internal function with "rte_"= =0A= > prefix; it should be reserved for public functions.=0A= >=0A= > --yliu=0A= >=0A= =0A=