RE: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn

["Gao Feng" <gfree.wind@foxmail.com>]

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