From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Abeni Subject: Re: [PATCH net-next 2/4] tc/act: remove unneeded RCU lock in action callback Date: Fri, 13 Jul 2018 16:26:56 +0200 Message-ID: <3d8ee4dbbc106ac7621a7789c003e46dd883932e.camel@redhat.com> References: <117cef422749024d62465c85c5d6e66c5b055360.1531473946.git.pabeni@redhat.com> <6a61aa2e-2653-eb9b-99c1-16566f353e77@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Jamal Hadi Salim , Cong Wang , Jiri Pirko , Alexei Starovoitov , Marcelo Ricardo Leitner To: Daniel Borkmann , netdev@vger.kernel.org Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:41668 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1729644AbeGMOlu (ORCPT ); Fri, 13 Jul 2018 10:41:50 -0400 In-Reply-To: <6a61aa2e-2653-eb9b-99c1-16566f353e77@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 2018-07-13 at 16:08 +0200, Daniel Borkmann wrote: > Hi Paolo, > > On 07/13/2018 11:55 AM, Paolo Abeni wrote: > > Each lockless action currently does its own RCU locking in ->act(). > > This is allows using plain RCU accessor, even if the context > > is really RCU BH. > > > > This change drops the per action RCU lock, replace the accessors > > with _bh variant, cleans up a bit the surronding code and documents > > the RCU status in the relevant header. > > No functional nor performance change is intended. > > > > The goal of this patch is clarifying that the RCU critical section > > used by the tc actions extends up to the classifier's caller. > > > > Signed-off-by: Paolo Abeni > > [...] > > diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c > > index 06f743d8ed41..ac20266460c0 100644 > > --- a/net/sched/act_bpf.c > > +++ b/net/sched/act_bpf.c > > @@ -45,8 +45,7 @@ static int tcf_bpf(struct sk_buff *skb, const struct tc_action *act, > > tcf_lastuse_update(&prog->tcf_tm); > > bstats_cpu_update(this_cpu_ptr(prog->common.cpu_bstats), skb); > > > > - rcu_read_lock(); > > - filter = rcu_dereference(prog->filter); > > + filter = rcu_dereference_bh(prog->filter); > > if (at_ingress) { > > __skb_push(skb, skb->mac_len); > > bpf_compute_data_pointers(skb); > > @@ -56,7 +55,6 @@ static int tcf_bpf(struct sk_buff *skb, const struct tc_action *act, > > bpf_compute_data_pointers(skb); > > filter_res = BPF_PROG_RUN(filter, skb); > > } > > - rcu_read_unlock(); > > This conversion is not correct, BPF itself relies on RCU but not RCU-bh flavor. > You might probably see a splat if you do e.g. a map lookup with this change in > interpreter mode on tx side. Thank you for your review. I actually tested with lockdep, and lockdep is happy about it. The not so nice fact is that many TC modules already use plain RCU primitives in the control path (call_rcu, kfree_rcu, etc.) and rcu_derefence_bh() in the datapath (e.g. all the classifiers). AFACS, despite the mix, this use is safe. Cheers, Paolo