* [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
@ 2017-03-22 2:15 gfree.wind
2017-03-29 10:07 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: gfree.wind @ 2017-03-22 2:15 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: Gao Feng
From: Gao Feng <fgao@ikuai8.com>
Because these two functions return the nf_ct_helper_expectfn pointer
which should be protected by rcu lock. So it should makes sure the
caller should hold the rcu lock, not inside these functions.
Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
v2: Shorter subject, per Pablo
v1: Initial version
net/netfilter/nf_conntrack_helper.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..bce3d1f 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n)
}
EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
+/* Caller should hold the rcu lock */
struct nf_ct_helper_expectfn *
nf_ct_helper_expectfn_find_by_name(const char *name)
{
struct nf_ct_helper_expectfn *cur;
bool found = false;
- rcu_read_lock();
list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
if (!strcmp(cur->name, name)) {
found = true;
break;
}
}
- rcu_read_unlock();
return found ? cur : NULL;
}
EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
+/* Caller should hold the rcu lock */
struct nf_ct_helper_expectfn *
nf_ct_helper_expectfn_find_by_symbol(const void *symbol)
{
struct nf_ct_helper_expectfn *cur;
bool found = false;
- rcu_read_lock();
list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
if (cur->expectfn == symbol) {
found = true;
break;
}
}
- rcu_read_unlock();
return found ? cur : NULL;
}
EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_symbol);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
2017-03-22 2:15 [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn gfree.wind
@ 2017-03-29 10:07 ` Pablo Neira Ayuso
2017-03-29 10:29 ` Gao Feng
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-29 10:07 UTC (permalink / raw)
To: gfree.wind; +Cc: netfilter-devel, Gao Feng
On Wed, Mar 22, 2017 at 10:15:02AM +0800, gfree.wind@foxmail.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> Because these two functions return the nf_ct_helper_expectfn pointer
> which should be protected by rcu lock. So it should makes sure the
> caller should hold the rcu lock, not inside these functions.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> v2: Shorter subject, per Pablo
> v1: Initial version
>
> net/netfilter/nf_conntrack_helper.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 6dc44d9..bce3d1f 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n)
> }
> EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
>
> +/* Caller should hold the rcu lock */
> struct nf_ct_helper_expectfn *
> nf_ct_helper_expectfn_find_by_name(const char *name)
> {
> struct nf_ct_helper_expectfn *cur;
> bool found = false;
>
> - rcu_read_lock();
> list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
> if (!strcmp(cur->name, name)) {
> found = true;
> break;
> }
> }
> - rcu_read_unlock();
> return found ? cur : NULL;
> }
> EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
nf_ct_helper_expectfn_find_by_name() is called from ctnetlink, via
ctnetlink_create_expect() and rcu read side lock is not held there.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
2017-03-29 10:07 ` Pablo Neira Ayuso
@ 2017-03-29 10:29 ` Gao Feng
2017-03-29 10:44 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Gao Feng @ 2017-03-29 10:29 UTC (permalink / raw)
To: 'Pablo Neira Ayuso'; +Cc: netfilter-devel, 'Gao Feng'
Hi Pablo,
> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> Sent: Wednesday, March 29, 2017 6:08 PM
> To: gfree.wind@foxmail.com
> Cc: netfilter-devel@vger.kernel.org; Gao Feng <fgao@ikuai8.com>
> Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu
lock
> when get expectfn
>
> On Wed, Mar 22, 2017 at 10:15:02AM +0800, gfree.wind@foxmail.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > Because these two functions return the nf_ct_helper_expectfn pointer
> > which should be protected by rcu lock. So it should makes sure the
> > caller should hold the rcu lock, not inside these functions.
> >
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> > v2: Shorter subject, per Pablo
> > v1: Initial version
> >
> > net/netfilter/nf_conntrack_helper.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_helper.c
> > b/net/netfilter/nf_conntrack_helper.c
> > index 6dc44d9..bce3d1f 100644
> > --- a/net/netfilter/nf_conntrack_helper.c
> > +++ b/net/netfilter/nf_conntrack_helper.c
> > @@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct
> > nf_ct_helper_expectfn *n) }
> > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
> >
> > +/* Caller should hold the rcu lock */
> > struct nf_ct_helper_expectfn *
> > nf_ct_helper_expectfn_find_by_name(const char *name) {
> > struct nf_ct_helper_expectfn *cur;
> > bool found = false;
> >
> > - rcu_read_lock();
> > list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
> > if (!strcmp(cur->name, name)) {
> > found = true;
> > break;
> > }
> > }
> > - rcu_read_unlock();
> > return found ? cur : NULL;
> > }
> > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
>
> nf_ct_helper_expectfn_find_by_name() is called from ctnetlink, via
> ctnetlink_create_expect() and rcu read side lock is not held there.
There are two reasons.
1. The rcu lock would be added in my patch " netfilter: helper: Add the rcu
lock when call __nf_conntrack_helper_find" for nf
https://patchwork.ozlabs.org/patch/741865/.
So the ctnetlink_create_expect would hold the rcu lock after apply that
patch.
2. Because these two functions return one pointer which needs RCU lock, so
the caller must hold rcu lock.
Or it still meets one error even though there is one rcu lock in these two
functions.
Because the memory which the returned pointer point to would be freed
already after rcu_read_unlock.
So the rcu lock is unnecessary in these functions.
Best Regards
Feng
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
2017-03-29 10:29 ` Gao Feng
@ 2017-03-29 10:44 ` Pablo Neira Ayuso
2017-03-29 10:50 ` Gao Feng
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-29 10:44 UTC (permalink / raw)
To: Gao Feng; +Cc: netfilter-devel, 'Gao Feng'
On Wed, Mar 29, 2017 at 06:29:10PM +0800, Gao Feng wrote:
> Hi Pablo,
>
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > Sent: Wednesday, March 29, 2017 6:08 PM
> > To: gfree.wind@foxmail.com
> > Cc: netfilter-devel@vger.kernel.org; Gao Feng <fgao@ikuai8.com>
> > Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu
> lock
> > when get expectfn
> >
> > On Wed, Mar 22, 2017 at 10:15:02AM +0800, gfree.wind@foxmail.com wrote:
> > > From: Gao Feng <fgao@ikuai8.com>
> > >
> > > Because these two functions return the nf_ct_helper_expectfn pointer
> > > which should be protected by rcu lock. So it should makes sure the
> > > caller should hold the rcu lock, not inside these functions.
> > >
> > > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > > ---
> > > v2: Shorter subject, per Pablo
> > > v1: Initial version
> > >
> > > net/netfilter/nf_conntrack_helper.c | 6 ++----
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/netfilter/nf_conntrack_helper.c
> > > b/net/netfilter/nf_conntrack_helper.c
> > > index 6dc44d9..bce3d1f 100644
> > > --- a/net/netfilter/nf_conntrack_helper.c
> > > +++ b/net/netfilter/nf_conntrack_helper.c
> > > @@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct
> > > nf_ct_helper_expectfn *n) }
> > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
> > >
> > > +/* Caller should hold the rcu lock */
> > > struct nf_ct_helper_expectfn *
> > > nf_ct_helper_expectfn_find_by_name(const char *name) {
> > > struct nf_ct_helper_expectfn *cur;
> > > bool found = false;
> > >
> > > - rcu_read_lock();
> > > list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
> > > if (!strcmp(cur->name, name)) {
> > > found = true;
> > > break;
> > > }
> > > }
> > > - rcu_read_unlock();
> > > return found ? cur : NULL;
> > > }
> > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
> >
> > nf_ct_helper_expectfn_find_by_name() is called from ctnetlink, via
> > ctnetlink_create_expect() and rcu read side lock is not held there.
> There are two reasons.
> 1. The rcu lock would be added in my patch " netfilter: helper: Add the rcu
> lock when call __nf_conntrack_helper_find" for nf
> https://patchwork.ozlabs.org/patch/741865/.
If you have interdependencies between two patches like this, it's
better to make it in one single go.
> So the ctnetlink_create_expect would hold the rcu lock after apply that
> patch.
>
> 2. Because these two functions return one pointer which needs RCU lock, so
> the caller must hold rcu lock.
> Or it still meets one error even though there is one rcu lock in these two
> functions.
> Because the memory which the returned pointer point to would be freed
> already after rcu_read_unlock.
> So the rcu lock is unnecessary in these functions.
That's right. You're fixing up a real problem, no doubt.
I'm just questioning that I think that if you are fixing up rcu
locking, which seems to be the case, you just do it in one single
patch.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
2017-03-29 10:44 ` Pablo Neira Ayuso
@ 2017-03-29 10:50 ` Gao Feng
2017-03-29 11:49 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Gao Feng @ 2017-03-29 10:50 UTC (permalink / raw)
To: 'Pablo Neira Ayuso', 'Gao Feng'; +Cc: netfilter-devel
Hi Pablo,
> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> Sent: Wednesday, March 29, 2017 6:44 PM
> To: Gao Feng <gfree.wind@foxmail.com>
> Cc: netfilter-devel@vger.kernel.org; 'Gao Feng' <fgao@ikuai8.com>
> Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu
lock
> when get expectfn
>
> On Wed, Mar 29, 2017 at 06:29:10PM +0800, Gao Feng wrote:
> > Hi Pablo,
> >
> > > -----Original Message-----
> > > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > > Sent: Wednesday, March 29, 2017 6:08 PM
> > > To: gfree.wind@foxmail.com
> > > Cc: netfilter-devel@vger.kernel.org; Gao Feng <fgao@ikuai8.com>
> > > Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove
> > > useless rcu
> > lock
> > > when get expectfn
> > >
> > > On Wed, Mar 22, 2017 at 10:15:02AM +0800, gfree.wind@foxmail.com
> wrote:
> > > > From: Gao Feng <fgao@ikuai8.com>
> > > >
> > > > Because these two functions return the nf_ct_helper_expectfn
> > > > pointer which should be protected by rcu lock. So it should makes
> > > > sure the caller should hold the rcu lock, not inside these
functions.
> > > >
> > > > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > > > ---
> > > > v2: Shorter subject, per Pablo
> > > > v1: Initial version
> > > >
> > > > net/netfilter/nf_conntrack_helper.c | 6 ++----
> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/net/netfilter/nf_conntrack_helper.c
> > > > b/net/netfilter/nf_conntrack_helper.c
> > > > index 6dc44d9..bce3d1f 100644
> > > > --- a/net/netfilter/nf_conntrack_helper.c
> > > > +++ b/net/netfilter/nf_conntrack_helper.c
> > > > @@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct
> > > > nf_ct_helper_expectfn *n) }
> > > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
> > > >
> > > > +/* Caller should hold the rcu lock */
> > > > struct nf_ct_helper_expectfn *
> > > > nf_ct_helper_expectfn_find_by_name(const char *name) {
> > > > struct nf_ct_helper_expectfn *cur;
> > > > bool found = false;
> > > >
> > > > - rcu_read_lock();
> > > > list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list,
head) {
> > > > if (!strcmp(cur->name, name)) {
> > > > found = true;
> > > > break;
> > > > }
> > > > }
> > > > - rcu_read_unlock();
> > > > return found ? cur : NULL;
> > > > }
> > > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
> > >
> > > nf_ct_helper_expectfn_find_by_name() is called from ctnetlink, via
> > > ctnetlink_create_expect() and rcu read side lock is not held there.
> > There are two reasons.
> > 1. The rcu lock would be added in my patch " netfilter: helper: Add
> > the rcu lock when call __nf_conntrack_helper_find" for nf
> > https://patchwork.ozlabs.org/patch/741865/.
>
> If you have interdependencies between two patches like this, it's better
to
> make it in one single go.
>
> > So the ctnetlink_create_expect would hold the rcu lock after apply
> > that patch.
> >
> > 2. Because these two functions return one pointer which needs RCU
> > lock, so the caller must hold rcu lock.
> > Or it still meets one error even though there is one rcu lock in these
> > two functions.
> > Because the memory which the returned pointer point to would be freed
> > already after rcu_read_unlock.
> > So the rcu lock is unnecessary in these functions.
>
> That's right. You're fixing up a real problem, no doubt.
>
> I'm just questioning that I think that if you are fixing up rcu locking,
which
> seems to be the case, you just do it in one single patch.
>
> Thanks!
Ok, I would merge them into one patch.
Actually I couldn't get that what modifications could be done in the nf.
I learnt only bug fix could be accepted in net.git, so I assumed the nf.git
did too.
Best Regards
Feng
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn
2017-03-29 10:50 ` Gao Feng
@ 2017-03-29 11:49 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-29 11:49 UTC (permalink / raw)
To: Gao Feng; +Cc: netfilter-devel
On Wed, Mar 29, 2017 at 06:50:46PM +0800, Gao Feng wrote:
> Hi Pablo,
>
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > Sent: Wednesday, March 29, 2017 6:44 PM
> > To: Gao Feng <gfree.wind@foxmail.com>
> > Cc: netfilter-devel@vger.kernel.org; 'Gao Feng' <fgao@ikuai8.com>
> > Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu
> lock
> > when get expectfn
> >
> > On Wed, Mar 29, 2017 at 06:29:10PM +0800, Gao Feng wrote:
> > > Hi Pablo,
> > >
> > > > -----Original Message-----
> > > > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > > > Sent: Wednesday, March 29, 2017 6:08 PM
> > > > To: gfree.wind@foxmail.com
> > > > Cc: netfilter-devel@vger.kernel.org; Gao Feng <fgao@ikuai8.com>
> > > > Subject: Re: [PATCH nf-next v2 1/1] netfilter: helper: Remove
> > > > useless rcu
> > > lock
> > > > when get expectfn
> > > >
> > > > On Wed, Mar 22, 2017 at 10:15:02AM +0800, gfree.wind@foxmail.com
> > wrote:
> > > > > From: Gao Feng <fgao@ikuai8.com>
> > > > >
> > > > > Because these two functions return the nf_ct_helper_expectfn
> > > > > pointer which should be protected by rcu lock. So it should makes
> > > > > sure the caller should hold the rcu lock, not inside these
> functions.
> > > > >
> > > > > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > > > > ---
> > > > > v2: Shorter subject, per Pablo
> > > > > v1: Initial version
> > > > >
> > > > > net/netfilter/nf_conntrack_helper.c | 6 ++----
> > > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/net/netfilter/nf_conntrack_helper.c
> > > > > b/net/netfilter/nf_conntrack_helper.c
> > > > > index 6dc44d9..bce3d1f 100644
> > > > > --- a/net/netfilter/nf_conntrack_helper.c
> > > > > +++ b/net/netfilter/nf_conntrack_helper.c
> > > > > @@ -311,38 +311,36 @@ void nf_ct_helper_expectfn_unregister(struct
> > > > > nf_ct_helper_expectfn *n) }
> > > > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
> > > > >
> > > > > +/* Caller should hold the rcu lock */
> > > > > struct nf_ct_helper_expectfn *
> > > > > nf_ct_helper_expectfn_find_by_name(const char *name) {
> > > > > struct nf_ct_helper_expectfn *cur;
> > > > > bool found = false;
> > > > >
> > > > > - rcu_read_lock();
> > > > > list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list,
> head) {
> > > > > if (!strcmp(cur->name, name)) {
> > > > > found = true;
> > > > > break;
> > > > > }
> > > > > }
> > > > > - rcu_read_unlock();
> > > > > return found ? cur : NULL;
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
> > > >
> > > > nf_ct_helper_expectfn_find_by_name() is called from ctnetlink, via
> > > > ctnetlink_create_expect() and rcu read side lock is not held there.
> > > There are two reasons.
> > > 1. The rcu lock would be added in my patch " netfilter: helper: Add
> > > the rcu lock when call __nf_conntrack_helper_find" for nf
> > > https://patchwork.ozlabs.org/patch/741865/.
> >
> > If you have interdependencies between two patches like this, it's better
> to
> > make it in one single go.
> >
> > > So the ctnetlink_create_expect would hold the rcu lock after apply
> > > that patch.
> > >
> > > 2. Because these two functions return one pointer which needs RCU
> > > lock, so the caller must hold rcu lock.
> > > Or it still meets one error even though there is one rcu lock in these
> > > two functions.
> > > Because the memory which the returned pointer point to would be freed
> > > already after rcu_read_unlock.
> > > So the rcu lock is unnecessary in these functions.
> >
> > That's right. You're fixing up a real problem, no doubt.
> >
> > I'm just questioning that I think that if you are fixing up rcu locking,
> which
> > seems to be the case, you just do it in one single patch.
> >
> > Thanks!
>
> Ok, I would merge them into one patch.
> Actually I couldn't get that what modifications could be done in the nf.
> I learnt only bug fix could be accepted in net.git, so I assumed the nf.git
> did too.
Yes, fixes go to nf.git and updates go to nf-next.git.
But in this case this has to go through nf.git since both patches
(that should be one) are part of the same logical changes since they
are required to fix up the RCU read side locking there consistently.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-29 11:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-22 2:15 [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn gfree.wind
2017-03-29 10:07 ` Pablo Neira Ayuso
2017-03-29 10:29 ` Gao Feng
2017-03-29 10:44 ` Pablo Neira Ayuso
2017-03-29 10:50 ` Gao Feng
2017-03-29 11:49 ` Pablo Neira Ayuso
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.