* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).