| Message ID | 1427115225-14489-1-git-send-email-pboldin@mirantis.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 79BAD3796; Mon, 23 Mar 2015 13:53:33 +0100 (CET) Received: from mail-wg0-f54.google.com (mail-wg0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 5BD88A69 for <dev@dpdk.org>; Mon, 23 Mar 2015 13:53:32 +0100 (CET) Received: by wgra20 with SMTP id a20so145121501wgr.3 for <dev@dpdk.org>; Mon, 23 Mar 2015 05:53:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mirantis.com; s=google; h=from:to:subject:date:message-id; bh=DWB075lg4dVWtgH8njsAQH7HWVOJx4y2IfjBPh6BLC0=; b=YaFL5EGIm2C8yJWKxsRmaZ6hWJXUMVsd7WvlBFIiVagpNxkjPvHWceO/0aHuvWS4VI IiUYxoA1PT5pF3zW+Pk0Q9kyIJgPfUoC1aU7R7pLc0plJ+ii3TP10UnNltqLIyhUMtDO RFpPfAZa4nbH13lpbDHmi2Mka+8JEyc8MifEY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id; bh=DWB075lg4dVWtgH8njsAQH7HWVOJx4y2IfjBPh6BLC0=; b=N9mtEEa8Q31CNGy4Ca7lptYj3xjw0qmOb3QUXTxLuHS8swKfk3orqQw55NCnCWxhkO 1FECQsrR/GSs80HfdeLsdkguuUX5sagqfJZoqNP6cE0tb23bKM3jn5bRkDbrdU/3+jUD 8dF8lJ5IDqXCAcOVrr/TeZVbeFa/XptDsZxqV9zJRtqLZpcJbfSrJh9hNB6ld1dF/LvZ 6Y5naT5SL2xsAn1CZaAZerXVLgxOt3MC6RBxz4pjIwHxBFsiLyL+55wOzgYzYsfUPy0u CwYc0efsmx9ei7QqQQXf6Bw0FYwYBCnC078iZ9YNx7Zn8heAVvVzxMEg/CErHEUjz+wA KYvA== X-Gm-Message-State: ALoCoQnur/eBJT62T7RznDOUlDcoq5+qWctBV4c6qh/W+Ip4yEkCfEycpiTtD4Jn5MjjcsfhzUVS X-Received: by 10.180.219.107 with SMTP id pn11mr14978624wic.6.1427115212184; Mon, 23 Mar 2015 05:53:32 -0700 (PDT) Received: from pboldin-pc.kha.mirantis.net ([194.213.110.67]) by mx.google.com with ESMTPSA id hs8sm1091196wib.11.2015.03.23.05.53.31 for <dev@dpdk.org> (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 23 Mar 2015 05:53:31 -0700 (PDT) From: Pavel Boldin <pboldin@mirantis.com> To: dev@dpdk.org Date: Mon, 23 Mar 2015 14:53:45 +0200 Message-Id: <1427115225-14489-1-git-send-email-pboldin@mirantis.com> X-Mailer: git-send-email 1.9.1 Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Pavel Boldin
March 23, 2015, 12:53 p.m. UTC
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 <pboldin@mirantis.com>
---
lib/librte_vhost/eventfd_link/eventfd_link.c | 1 +
1 file changed, 1 insertion(+)
Comments
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 <pboldin@mirantis.com> > --- > 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? > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL;
On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com> 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 <pboldin@mirantis.com> > > --- > > 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. Pavel > filp_close(file, files); > > fdt = files_fdtable(files); > > fdt->fd[eventfd_copy.source_fd] = NULL; > >
On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com>> > --- > 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? Pavel > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL;
On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com>> 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 <pboldin@mirantis.com<mailto: > pboldin@mirantis.com>> > > --- > > 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'. 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; > > > >
On 3/23/2015 10:52 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com>>> > --- > 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;
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 <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com>>> > > --- > > 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; >
On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > On 3/23/2015 10:52 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto: > huawei.xie@intel.com>>> 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 <pboldin@mirantis.com<mailto: > pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto: > pboldin@mirantis.com>>> > > --- > > 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. > The code of this module received little to none review and requires some love at the moment. I wanted to refactor the module completely but Thomas said it is not going to go into the 2.0. So I decided to make a simple one-line fix. If you are interested this [0] is the latest version of the refactoring patch. I can provide you with an application that checks that there is indeed no leakage and ensures that the `eventfd' moving works. It is being used in our builds as a test [1]. The code is "heredoc"ed in [2] [0] http://dpdk.org/dev/patchwork/patch/4113/ [1] https://review.fuel-infra.org/#/c/4639/ [2] https://review.fuel-infra.org/#/c/4639/3/tests/runtests.sh Pavel > 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; > > > > > >
On 3/23/2015 11:27 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: On 3/23/2015 10:52 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>> wrote: On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com>><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com>>>> > --- > 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. The code of this module received little to none review and requires some love at the moment. I wanted to refactor the module completely but Thomas said it is not going to go into the 2.0. So I decided to make a simple one-line fix. Another isse is do we really need a src fd here? Could we just allocate a unsed fd in the kernel and installed it with the target eventfd. If you are interested this [0] is the latest version of the refactoring patch. I can provide you with an application that checks that there is indeed no leakage and ensures that the `eventfd' moving works. It is being used in our builds as a test [1]. The code is "heredoc"ed in [2] [0] http://dpdk.org/dev/patchwork/patch/4113/ [1] https://review.fuel-infra.org/#/c/4639/ [2] https://review.fuel-infra.org/#/c/4639/3/tests/runtests.sh Pavel 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;
On Mon, Mar 23, 2015 at 5:36 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > On 3/23/2015 11:27 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com>> wrote: > On 3/23/2015 10:52 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto: > huawei.xie@intel.com>>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto: > huawei.xie@intel.com>><mailto:huawei.xie@intel.com<mailto: > huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto: > huawei.xie@intel.com>>>> 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 <pboldin@mirantis.com<mailto: > pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto: > pboldin@mirantis.com>><mailto:pboldin@mirantis.com<mailto: > pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto: > pboldin@mirantis.com>>>> > > --- > > 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. > The code of this module received little to none review and requires some > love at the moment. > > I wanted to refactor the module completely but Thomas said it is not going > to go into the 2.0. So I decided to make a simple one-line fix. > Another isse is do we really need a src fd here? Could we just allocate a > unsed fd in the kernel and installed it with the target eventfd. > This is for DPDK team to decide and should be discussed separately. Pavel > > If you are interested this [0] is the latest version of the refactoring > patch. > > I can provide you with an application that checks that there is indeed no > leakage and ensures that the `eventfd' moving works. It is being used in > our builds as a test [1]. The code is "heredoc"ed in [2] > > [0] http://dpdk.org/dev/patchwork/patch/4113/ > [1] https://review.fuel-infra.org/#/c/4639/ > [2] https://review.fuel-infra.org/#/c/4639/3/tests/runtests.sh > > Pavel > > 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; > > > > > > > >
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 <pboldin@mirantis.com> > --- > 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); > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL; Acked-by Huawei Xie <huawei.xie@intel.com> In future, we should remove the allocation of src eventfd, allocate a free fd from kernel, and install it with target fd.
On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie@intel.com> 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 <pboldin@mirantis.com> > > --- > > 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); > > filp_close(file, files); > > fdt = files_fdtable(files); > > fdt->fd[eventfd_copy.source_fd] = NULL; > Acked-by Huawei Xie <huawei.xie@intel.com> > > In future, we should remove the allocation of src eventfd, allocate a > free fd from kernel, and install it with target fd. > Well, I don't think this is correct, because this will put too much job for the kernel module making it a combiner. At the moment I propose to accept module refactoring patch to the upstream. After that the module can be reworked along with the application code. The reason is I can't work on DPDK since I have no hardware to test application at so I can't help with patch that touches application code. Pavel
On 3/24/2015 7:10 PM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com>> > --- > 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); > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL; Acked-by Huawei Xie <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> In future, we should remove the allocation of src eventfd, allocate a free fd from kernel, and install it with target fd. Well, I don't think this is correct, because this will put too much job for the kernel module making it a combiner. At the moment I propose to accept module refactoring patch to the upstream. After that the module can be reworked along with the application code. The reason is I can't work on DPDK since I have no hardware to test application at so I can't help with patch that touches application code. Pavel Pavel: I am not asking to do it right now but in future, and i agree we accept the refactoring patch now, so i ack the patch. Huawei
On Tue, Mar 24, 2015 at 6:20 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > On 3/24/2015 7:10 PM, Pavel Boldin wrote: > > > On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com>> 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 <pboldin@mirantis.com<mailto: > pboldin@mirantis.com>> > > --- > > 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); > > filp_close(file, files); > > fdt = files_fdtable(files); > > fdt->fd[eventfd_copy.source_fd] = NULL; > Acked-by Huawei Xie <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> > > In future, we should remove the allocation of src eventfd, allocate a > free fd from kernel, and install it with target fd. > > Well, I don't think this is correct, because this will put too much job > for the kernel module making it a combiner. > > At the moment I propose to accept module refactoring patch to the upstream. > > After that the module can be reworked along with the application code. The > reason is I can't work on DPDK since I have no hardware to test application > at so I can't help with patch that touches application code. > > Pavel > Pavel: > I am not asking to do it right now but in future, and i agree we accept > the refactoring patch now, so i ack the patch. > Huawei, this is the refactoring patch [1]. This patch is merely a bugfix. [1] http://dpdk.org/dev/patchwork/patch/4113/ > Huawei >
On 3/25/2015 4:08 AM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 6:20 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: On 3/24/2015 7:10 PM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>> 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 <pboldin@mirantis.com<mailto:pboldin@mirantis.com><mailto:pboldin@mirantis.com<mailto:pboldin@mirantis.com>>> > --- > 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); > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL; Acked-by Huawei Xie <huawei.xie@intel.com<mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com<mailto:huawei.xie@intel.com>>> In future, we should remove the allocation of src eventfd, allocate a free fd from kernel, and install it with target fd. Well, I don't think this is correct, because this will put too much job for the kernel module making it a combiner. At the moment I propose to accept module refactoring patch to the upstream. After that the module can be reworked along with the application code. The reason is I can't work on DPDK since I have no hardware to test application at so I can't help with patch that touches application code. Pavel Pavel: I am not asking to do it right now but in future, and i agree we accept the refactoring patch now, so i ack the patch. Huawei, this is the refactoring patch [1]. This patch is merely a bugfix. [1] http://dpdk.org/dev/patchwork/patch/4113/ Huawei Thanks for your patch and fd checking tool. I will re-ack the patch as i see quoting issue in my previous ack.
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 <pboldin@mirantis.com> > --- > 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); > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL; Acked-by Huawei Xie <huawei.xie@intel.com>
On Thu, Mar 26, 2015 at 9:56 AM, Xie, Huawei <huawei.xie@intel.com> 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 <pboldin@mirantis.com> > > --- > > 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); > > filp_close(file, files); > > fdt = files_fdtable(files); > > fdt->fd[eventfd_copy.source_fd] = NULL; > > Acked-by Huawei Xie <huawei.xie@intel.com> > The patch is still in "New" stat in patchwork. I think you forgot a ":" between Acked-by and email. Pavel
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 <pboldin@mirantis.com> > --- > 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); > filp_close(file, files); > fdt = files_fdtable(files); > fdt->fd[eventfd_copy.source_fd] = NULL; Acked-by: Huawei Xie <huawei.xie@intel.com>
> > 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 <pboldin@mirantis.com> > > Acked-by Huawei Xie <huawei.xie@intel.com> Applied, thanks
> > 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 <pboldin@mirantis.com> > Acked-by Huawei Xie <huawei.xie@intel.com> Applied, thanks (already sent this notification but this ack was did many times in detached threads)
On Fri, Mar 27, 2015 at 1:10 PM, Thomas Monjalon <thomas.monjalon@6wind.com> 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 <pboldin@mirantis.com> > > > > Acked-by Huawei Xie <huawei.xie@intel.com> > > Applied, thanks > What should I do with the refactoring patch in your opinion? Should I split it in parts to ease review? Pavel
> > > > 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 <pboldin@mirantis.com> > > > > > > Acked-by Huawei Xie <huawei.xie@intel.com> > > > > Applied, thanks > > What should I do with the refactoring patch in your opinion? > > Should I split it in parts to ease review? I didn't review it so I have no good advice. But in general, it's better to split patches in logic units if it makes sense. Note that we are closing 2.0. Refactoring patches are deffered to 2.1.
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); filp_close(file, files); fdt = files_fdtable(files); fdt->fd[eventfd_copy.source_fd] = NULL;