From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xie, Huawei" Subject: Re: [PATCH] vhost: fix leak of fds and mmaps 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Yuanhan Liu , Rich Lane 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) Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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=