[dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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;

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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;

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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;

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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.

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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
>

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, 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.

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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>

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Pavel Boldin]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Thomas Monjalon]

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

Re: [dpdk-dev] [PATCH] vhost: Fix `struct file' leakage in `eventfd_link'

[Xie, Huawei]

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>