* [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-VfR2kkLFssw
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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>
---
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 related [flat|nested] 22+ messages in thread* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 12:53 [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin @ 2015-03-23 14:21 ` Xie, Huawei [not found] ` <C37D651A908B024F974696C65296B57B0F3F1E14-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 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-VfR2kkLFssw 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > --- > 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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F1E14-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F1E14-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > --- > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 14:36 ` Pavel Boldin @ 2015-03-23 14:41 ` Xie, Huawei [not found] ` <C37D651A908B024F974696C65296B57B0F3F1F75-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-23 14:41 UTC (permalink / raw) To: Pavel Boldin; +Cc: dev-VfR2kkLFssw On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F1F75-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F1F75-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>> > > --- > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 14:51 ` Pavel Boldin @ 2015-03-23 15:16 ` Xie, Huawei [not found] ` <C37D651A908B024F974696C65296B57B0F3F240D-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-23 15:16 UTC (permalink / raw) To: Pavel Boldin; +Cc: dev-VfR2kkLFssw On 3/23/2015 10:52 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuE@public.gmane.orgm>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin@mirantis.com><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>> > --- > 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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F240D-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F240D-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>> > > --- > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F240D-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 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-VfR2kkLFssw On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote: > On 3/23/2015 10:52 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>> > > --- > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 15:27 ` Pavel Boldin @ 2015-03-23 15:36 ` Xie, Huawei [not found] ` <C37D651A908B024F974696C65296B57B0F3F2694-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-23 15:36 UTC (permalink / raw) To: Pavel Boldin; +Cc: dev-VfR2kkLFssw On 3/23/2015 11:27 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: On 3/23/2015 10:52 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuE@public.gmane.orgm>>> wrote: On 3/23/2015 10:37 PM, Pavel Boldin wrote: On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuE@public.gmane.orgm>><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin@mirantis.com><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>>> > --- > 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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F2694-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F2694-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Mon, Mar 23, 2015 at 5:36 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote: > On 3/23/2015 11:27 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 5:16 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: > On 3/23/2015 10:52 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:41 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>> wrote: > On 3/23/2015 10:37 PM, Pavel Boldin wrote: > > > On Mon, Mar 23, 2015 at 4:21 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>>> > > --- > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 12:53 [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 [not found] ` <C37D651A908B024F974696C65296B57B0F3F46FB-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 2015-03-26 7:56 ` Xie, Huawei 2015-03-26 15:20 ` Xie, Huawei 3 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-24 6:28 UTC (permalink / raw) To: Pavel Boldin, dev-VfR2kkLFssw, 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > --- > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F46FB-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F46FB-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > --- > > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-24 11:10 ` Pavel Boldin @ 2015-03-24 16:20 ` Xie, Huawei [not found] ` <C37D651A908B024F974696C65296B57B0F3F59D7-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-24 16:20 UTC (permalink / raw) To: Pavel Boldin; +Cc: dev-VfR2kkLFssw On 3/24/2015 7:10 PM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> 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
[parent not found: <C37D651A908B024F974696C65296B57B0F3F59D7-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F59D7-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Tue, Mar 24, 2015 at 6:20 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote: > On 3/24/2015 7:10 PM, Pavel Boldin wrote: > > > On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto: > huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto: > pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>> > > --- > > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> > > 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: [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-VfR2kkLFssw On 3/25/2015 4:08 AM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 6:20 PM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote: On 3/24/2015 7:10 PM, Pavel Boldin wrote: On Tue, Mar 24, 2015 at 8:28 AM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuE@public.gmane.orgm>>> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin@mirantis.com><mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org<mailto:pboldin-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org>>> > --- > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org><mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org<mailto:huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>>> 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3F46FB-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 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-VfR2kkLFssw > > 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > Acked-by Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 12:53 [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 [not found] ` <C37D651A908B024F974696C65296B57B0F3FB942-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 2015-03-26 15:20 ` Xie, Huawei 3 siblings, 1 reply; 22+ messages in thread From: Xie, Huawei @ 2015-03-26 7:56 UTC (permalink / raw) To: Pavel Boldin, dev-VfR2kkLFssw 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > --- > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <C37D651A908B024F974696C65296B57B0F3FB942-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3FB942-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 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-VfR2kkLFssw On Thu, Mar 26, 2015 at 9:56 AM, Xie, Huawei <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > --- > > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > 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: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <C37D651A908B024F974696C65296B57B0F3FB942-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org> 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-VfR2kkLFssw > > 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > Acked-by Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Applied, thanks ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-27 11:10 ` Thomas Monjalon @ 2015-03-27 12:36 ` Pavel Boldin [not found] ` <CACf4_B9ims0bL3fqk5Pm6sTHkm7pdHcTgFNXLvKYZf5Gip9LDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Pavel Boldin @ 2015-03-27 12:36 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev-VfR2kkLFssw On Fri, Mar 27, 2015 at 1:10 PM, Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > > > Acked-by Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > 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
[parent not found: <CACf4_B9ims0bL3fqk5Pm6sTHkm7pdHcTgFNXLvKYZf5Gip9LDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' [not found] ` <CACf4_B9ims0bL3fqk5Pm6sTHkm7pdHcTgFNXLvKYZf5Gip9LDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 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-VfR2kkLFssw > > > > 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > > > > > > Acked-by Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > > > > 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
* Re: [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' 2015-03-23 12:53 [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-VfR2kkLFssw 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-nYU0QVwCCFFWk0Htik3J/w@public.gmane.org> > --- > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-03-27 13:15 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 [PATCH] vhost: Fix `struct file' leakage in `eventfd_link' Pavel Boldin
2015-03-23 14:21 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F1E14-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-23 14:36 ` Pavel Boldin
2015-03-23 14:41 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F1F75-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-23 14:51 ` Pavel Boldin
2015-03-23 15:16 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F240D-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-23 15:22 ` Thomas Monjalon
2015-03-23 15:27 ` Pavel Boldin
2015-03-23 15:36 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F2694-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-23 15:44 ` Pavel Boldin
2015-03-24 6:28 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F46FB-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-24 11:10 ` Pavel Boldin
2015-03-24 16:20 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B0F3F59D7-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
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
[not found] ` <C37D651A908B024F974696C65296B57B0F3FB942-0J0gbvR4kThpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2015-03-26 15:17 ` Pavel Boldin
2015-03-27 11:10 ` Thomas Monjalon
2015-03-27 12:36 ` Pavel Boldin
[not found] ` <CACf4_B9ims0bL3fqk5Pm6sTHkm7pdHcTgFNXLvKYZf5Gip9LDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-03-27 13:15 ` Thomas Monjalon
2015-03-26 15:20 ` Xie, Huawei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.