* [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.
@ 2017-03-14 8:29 fgao
2017-03-14 8:45 ` Feng Gao
2017-03-21 14:30 ` Pablo Neira Ayuso
0 siblings, 2 replies; 4+ messages in thread
From: fgao @ 2017-03-14 8:29 UTC (permalink / raw)
To: pablo, netfilter-devel, gfree.wind; +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>
---
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] 4+ messages in thread* 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.
2017-03-14 8:29 [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 fgao
@ 2017-03-14 8:45 ` Feng Gao
2017-03-21 14:30 ` Pablo Neira Ayuso
1 sibling, 0 replies; 4+ messages in thread
From: Feng Gao @ 2017-03-14 8:45 UTC (permalink / raw)
To: Pablo Neira Ayuso, Netfilter Developer Mailing List, Feng Gao; +Cc: Gao Feng
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
^ permalink raw reply [flat|nested] 4+ messages in thread* 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.
2017-03-14 8:29 [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 fgao
2017-03-14 8:45 ` Feng Gao
@ 2017-03-21 14:30 ` Pablo Neira Ayuso
2017-03-21 14:36 ` 高峰
1 sibling, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-21 14:30 UTC (permalink / raw)
To: fgao; +Cc: netfilter-devel, gfree.wind
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* 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.
2017-03-21 14:30 ` Pablo Neira Ayuso
@ 2017-03-21 14:36 ` 高峰
0 siblings, 0 replies; 4+ messages in thread
From: 高峰 @ 2017-03-21 14:36 UTC (permalink / raw)
To: 'Pablo Neira Ayuso'; +Cc: netfilter-devel, gfree.wind
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-21 14:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-14 8:29 [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 fgao
2017-03-14 8:45 ` Feng Gao
2017-03-21 14:30 ` Pablo Neira Ayuso
2017-03-21 14:36 ` 高峰
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).