From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gao Feng" Subject: RE: [PATCH nf-next v2 1/1] netfilter: helper: Remove useless rcu lock when get expectfn Date: Wed, 29 Mar 2017 18:29:10 +0800 Message-ID: <000301d2a877$4e174c50$ea45e4f0$@foxmail.com> References: <1490148902-96508-1-git-send-email-gfree.wind@foxmail.com> <20170329100730.GA4131@salvia> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: , "'Gao Feng'" To: "'Pablo Neira Ayuso'" Return-path: Received: from smtpbgau1.qq.com ([54.206.16.166]:39019 "EHLO smtpbgau1.qq.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755295AbdC2K3g (ORCPT ); Wed, 29 Mar 2017 06:29:36 -0400 In-Reply-To: <20170329100730.GA4131@salvia> Content-Language: zh-cn Sender: netfilter-devel-owner@vger.kernel.org List-ID: 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 > 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 > > > > 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 > > --- > > 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