From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 2496E5957 for ; Mon, 23 Mar 2015 16:23:31 +0100 (CET) Received: by wgbcc7 with SMTP id cc7so148670797wgb.0 for ; Mon, 23 Mar 2015 08:23:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=UCFLX8ba140j2HbZdeRyKfwZg51k7o7V512iB5rEXgM=; b=RM+4HObY5Xk3PGccJmTuAsLLrFwVuLIPKsM1T62aDQ2nU9/VNQTWaUi2XGZqlSJKBS m0Gl+NuoGyt1yywur4S+uuwArxV+T7c2nNN19CaH1gCUl3g9Qs9DW+/qIbX9H5gN3TGA FJ6zmIWhf3fSVdACp18xtUJ0b5Ypmp1db+3KTelqhHZMuLub6+EFM81gi4yDgdeUm5KG MT0dIHZt/IYnuMTlS9wrhqQ0ENezzSqUcvRje38KTPnzMJ6yCtY9SPvwm/n/tsXCyGlF jva59A4gvrjOFpvmcbf2L43LbdyAMIuwer9VaxKwAq1phK8YQy0BF6GkMSJYIpxLrgKa p0cg== X-Gm-Message-State: ALoCoQnatbPYiX148qsCfNH5CgthVPn//wUo3B6Qp1a1G8dos/FW6ilvS9W/Pbg6lXQObaRKiRyn X-Received: by 10.194.175.137 with SMTP id ca9mr120064403wjc.67.1427124210997; Mon, 23 Mar 2015 08:23:30 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id lu13sm11600095wic.10.2015.03.23.08.23.29 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 Mar 2015 08:23:30 -0700 (PDT) From: Thomas Monjalon To: "Xie, Huawei" Date: Mon, 23 Mar 2015 16:22:49 +0100 Message-ID: <4710593.KA6H5ovzJI@xps13> Organization: 6WIND User-Agent: KMail/4.14.4 (Linux/3.18.4-1-ARCH; KDE/4.14.4; x86_64; ; ) In-Reply-To: References: <1427115225-14489-1-git-send-email-pboldin@mirantis.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 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: Mon, 23 Mar 2015 15:23:31 -0000 Huawei, This thread is unreadable because your mailer is not quoting. Please check. Thanks 2015-03-23 15:16, Xie, Huawei: > On 3/23/2015 10:52 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei > wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei >> wrote: > On 3/23/2015 8:54 PM, Pavel Boldin wrote: > > Due to increased `struct file's reference counter subsequent call > > to `filp_close' does not free the `struct file'. Prepend `fput' call > > to decrease the reference counter. > > > > Signed-off-by: Pavel Boldin >> > > --- > > lib/librte_vhost/eventfd_link/eventfd_link.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c > > index 7755dd6..62c45c8 100644 > > --- a/lib/librte_vhost/eventfd_link/eventfd_link.c > > +++ b/lib/librte_vhost/eventfd_link/eventfd_link.c > > @@ -117,6 +117,7 @@ eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg) > > * Release the existing eventfd in the source process > > */ > > spin_lock(&files->file_lock); > > + fput(file); > Could we just call atomic_long_dec here? > > We can but I don't like breaking encapsulation (which is broken anyway by the code). So, there is a special method and we should use it in my opinion. > it is increased by atomic_long_inc_not_zero so why don't we use the symmetric function? > The code with `atomic_long_inc_not_zero' call is a copy-paste of the `fget' function. If we want to make it clear we should make a separate function and name it so: `fget_from_files'. > > I don't understand why there is a (exact?) copy&paste of fget there. :). > Maybe you could post a patchset, first replace the copy/paste with fget and then this patch. It will looks much clearer. > Second thing is: another thread of the same processor can call the `sys_close' on the `fd' and this will dereference counter so `fput' will correctly free the `struct file'. Using `atomic_long_dec' will leak a `struct file' and print a KERN_ERR message by `filp_close'. > > So, the common thing is to use appropriate functions and don't reinvent the wheel. > > Pavel > > > > Pavel > > > filp_close(file, files); > > fdt = files_fdtable(files); > > fdt->fd[eventfd_copy.source_fd] = NULL; >