| Message ID | 1489480145-54411-1-git-send-email-fgao@ikuai8.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Hi Palbo, On Tue, Mar 14, 2017 at 4:29 PM, <fgao@ikuai8.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> > --- > 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 > > Sorry, I misspelled the title. It should be "nf-next", not "net-next". Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 14, 2017 at 04:29:05PM +0800, fgao@ikuai8.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> > --- > 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); You have to collapse this patch to: http://patchwork.ozlabs.org/patch/740576/ Please... use shorter patch subject names, around 80 chars long. There is no strict limit that I know, but this subject looks too long. I think rcu read side is missing in every invocations to: __nf_conntrack_helper_find() in ctnetlink. So this patch would be larger, have a closer look and fix this in one go, please. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo, > -----Original Message----- > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org] > Sent: Tuesday, March 21, 2017 10:30 PM > To: fgao@ikuai8.com > Cc: netfilter-devel@vger.kernel.org; gfree.wind@gmail.com > Subject: Re: [PATCH net-next 1/1] netfilter: helper: Remove the rcu lock in > nf_ct_helper_expectfn_find_by_name and > nf_ct_helper_expectfn_find_by_symbol. > > On Tue, Mar 14, 2017 at 04:29:05PM +0800, fgao@ikuai8.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> > > --- > > 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); > > You have to collapse this patch to: > > http://patchwork.ozlabs.org/patch/740576/ > > Please... use shorter patch subject names, around 80 chars long. There is no > strict limit that I know, but this subject looks too long. Ok. I would make it shorter, and send another update. > > I think rcu read side is missing in every invocations to: > > __nf_conntrack_helper_find() > > in ctnetlink. So this patch would be larger, have a closer look and fix this in one > go, please. No problem, I would check all callers. Best Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);