DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
@ 2015-03-23 12:53 Pavel Boldin
  2015-03-23 14:21 ` Xie, Huawei
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Pavel Boldin @ 2015-03-23 12:53 UTC (permalink / raw)
  To: dev

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;
-- 
1.9.1

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 12:53 [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
@ 2015-03-23 14:21 ` Xie, Huawei
  2015-03-23 14:36   ` Pavel Boldin
  2015-03-24  6:28 ` Xie, Huawei
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Xie, Huawei @ 2015-03-23 14:21 UTC (permalink / raw)
  To: Pavel Boldin, dev

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;


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 14:21 ` Xie, Huawei
@ 2015-03-23 14:36   ` Pavel Boldin
  2015-03-23 14:41     ` Xie, Huawei
  0 siblings, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-23 14:36 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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;
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 14:36   ` Pavel Boldin
@ 2015-03-23 14:41     ` Xie, Huawei
  2015-03-23 14:51       ` Pavel Boldin
  0 siblings, 1 reply; 22+ messages in thread
From: Xie, Huawei @ 2015-03-23 14:41 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 14:41     ` Xie, Huawei
@ 2015-03-23 14:51       ` Pavel Boldin
  2015-03-23 15:16         ` Xie, Huawei
  0 siblings, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-23 14:51 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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;
>
>
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 14:51       ` Pavel Boldin
@ 2015-03-23 15:16         ` Xie, Huawei
  2015-03-23 15:22           ` Thomas Monjalon
  2015-03-23 15:27           ` Pavel Boldin
  0 siblings, 2 replies; 22+ messages in thread
From: Xie, Huawei @ 2015-03-23 15:16 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 15:16         ` Xie, Huawei
@ 2015-03-23 15:22           ` Thomas Monjalon
  2015-03-23 15:27           ` Pavel Boldin
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Monjalon @ 2015-03-23 15:22 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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;
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 15:16         ` Xie, Huawei
  2015-03-23 15:22           ` Thomas Monjalon
@ 2015-03-23 15:27           ` Pavel Boldin
  2015-03-23 15:36             ` Xie, Huawei
  1 sibling, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-23 15:27 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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;
>
>
>
>
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 15:27           ` Pavel Boldin
@ 2015-03-23 15:36             ` Xie, Huawei
  2015-03-23 15:44               ` Pavel Boldin
  0 siblings, 1 reply; 22+ messages in thread
From: Xie, Huawei @ 2015-03-23 15:36 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 15:36             ` Xie, Huawei
@ 2015-03-23 15:44               ` Pavel Boldin
  0 siblings, 0 replies; 22+ messages in thread
From: Pavel Boldin @ 2015-03-23 15:44 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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;
>
>
>
>
>
>
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 12:53 [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
  2015-03-23 14:21 ` Xie, Huawei
@ 2015-03-24  6:28 ` Xie, Huawei
  2015-03-24 11:10   ` Pavel Boldin
  2015-03-27 11:12   ` Thomas Monjalon
  2015-03-26  7:56 ` Xie, Huawei
  2015-03-26 15:20 ` Xie, Huawei
  3 siblings, 2 replies; 22+ messages in thread
From: Xie, Huawei @ 2015-03-24  6:28 UTC (permalink / raw)
  To: Pavel Boldin, dev, Long, Thomas

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-24  6:28 ` Xie, Huawei
@ 2015-03-24 11:10   ` Pavel Boldin
  2015-03-24 16:20     ` Xie, Huawei
  2015-03-27 11:12   ` Thomas Monjalon
  1 sibling, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-24 11:10 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-24 11:10   ` Pavel Boldin
@ 2015-03-24 16:20     ` Xie, Huawei
  2015-03-24 20:08       ` Pavel Boldin
  0 siblings, 1 reply; 22+ messages in thread
From: Xie, Huawei @ 2015-03-24 16:20 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-24 16:20     ` Xie, Huawei
@ 2015-03-24 20:08       ` Pavel Boldin
  2015-03-26  7:53         ` Xie, Huawei
  0 siblings, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-24 20:08 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-24 20:08       ` Pavel Boldin
@ 2015-03-26  7:53         ` Xie, Huawei
  0 siblings, 0 replies; 22+ messages in thread
From: Xie, Huawei @ 2015-03-26  7:53 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 12:53 [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
  2015-03-23 14:21 ` Xie, Huawei
  2015-03-24  6:28 ` Xie, Huawei
@ 2015-03-26  7:56 ` Xie, Huawei
  2015-03-26 15:17   ` Pavel Boldin
  2015-03-27 11:10   ` Thomas Monjalon
  2015-03-26 15:20 ` Xie, Huawei
  3 siblings, 2 replies; 22+ messages in thread
From: Xie, Huawei @ 2015-03-26  7:56 UTC (permalink / raw)
  To: Pavel Boldin, dev

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>



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-26  7:56 ` Xie, Huawei
@ 2015-03-26 15:17   ` Pavel Boldin
  2015-03-27 11:10   ` Thomas Monjalon
  1 sibling, 0 replies; 22+ messages in thread
From: Pavel Boldin @ 2015-03-26 15:17 UTC (permalink / raw)
  To: Xie, Huawei; +Cc: dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-23 12:53 [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
                   ` (2 preceding siblings ...)
  2015-03-26  7:56 ` Xie, Huawei
@ 2015-03-26 15:20 ` Xie, Huawei
  3 siblings, 0 replies; 22+ messages in thread
From: Xie, Huawei @ 2015-03-26 15:20 UTC (permalink / raw)
  To: Pavel Boldin, dev

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>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-26  7:56 ` Xie, Huawei
  2015-03-26 15:17   ` Pavel Boldin
@ 2015-03-27 11:10   ` Thomas Monjalon
  2015-03-27 12:36     ` Pavel Boldin
  1 sibling, 1 reply; 22+ messages in thread
From: Thomas Monjalon @ 2015-03-27 11:10 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

> > 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-24  6:28 ` Xie, Huawei
  2015-03-24 11:10   ` Pavel Boldin
@ 2015-03-27 11:12   ` Thomas Monjalon
  1 sibling, 0 replies; 22+ messages in thread
From: Thomas Monjalon @ 2015-03-27 11:12 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

> > 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)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-27 11:10   ` Thomas Monjalon
@ 2015-03-27 12:36     ` Pavel Boldin
  2015-03-27 13:15       ` Thomas Monjalon
  0 siblings, 1 reply; 22+ messages in thread
From: Pavel Boldin @ 2015-03-27 12:36 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'
  2015-03-27 12:36     ` Pavel Boldin
@ 2015-03-27 13:15       ` Thomas Monjalon
  0 siblings, 0 replies; 22+ messages in thread
From: Thomas Monjalon @ 2015-03-27 13:15 UTC (permalink / raw)
  To: Pavel Boldin; +Cc: dev

> > > > 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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2015-03-27 13:16 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-23 12:53 [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
2015-03-23 14:21 ` Xie, Huawei
2015-03-23 14:36   ` Pavel Boldin
2015-03-23 14:41     ` Xie, Huawei
2015-03-23 14:51       ` Pavel Boldin
2015-03-23 15:16         ` Xie, Huawei
2015-03-23 15:22           ` Thomas Monjalon
2015-03-23 15:27           ` Pavel Boldin
2015-03-23 15:36             ` Xie, Huawei
2015-03-23 15:44               ` Pavel Boldin
2015-03-24  6:28 ` Xie, Huawei
2015-03-24 11:10   ` Pavel Boldin
2015-03-24 16:20     ` Xie, Huawei
2015-03-24 20:08       ` Pavel Boldin
2015-03-26  7:53         ` Xie, Huawei
2015-03-27 11:12   ` Thomas Monjalon
2015-03-26  7:56 ` Xie, Huawei
2015-03-26 15:17   ` Pavel Boldin
2015-03-27 11:10   ` Thomas Monjalon
2015-03-27 12:36     ` Pavel Boldin
2015-03-27 13:15       ` Thomas Monjalon
2015-03-26 15:20 ` Xie, Huawei

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).