From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback Date: Tue, 30 Sep 2014 18:18:49 -0700 Message-ID: <542B5679.3060601@gmail.com> References: <1412118444-29179-1-git-send-email-xiyou.wangcong@gmail.com> <1412118444-29179-2-git-send-email-xiyou.wangcong@gmail.com> <542B5096.2040106@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, davem@davemloft.net, John Fastabend To: Cong Wang Return-path: Received: from mail-ob0-f182.google.com ([209.85.214.182]:47317 "EHLO mail-ob0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750981AbaJABTD (ORCPT ); Tue, 30 Sep 2014 21:19:03 -0400 Received: by mail-ob0-f182.google.com with SMTP id uy5so104390obc.13 for ; Tue, 30 Sep 2014 18:19:03 -0700 (PDT) In-Reply-To: <542B5096.2040106@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 09/30/2014 05:53 PM, John Fastabend wrote: > On 09/30/2014 04:07 PM, Cong Wang wrote: >> This fixes the following crash: >> >> [ 63.976822] general protection fault: 0000 [#1] PREEMPT SMP >> DEBUG_PAGEALLOC >> [ 63.980094] CPU: 1 PID: 15 Comm: ksoftirqd/1 Not tainted >> 3.17.0-rc6+ #648 >> [ 63.980094] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 >> [ 63.980094] task: ffff880117dea690 ti: ffff880117dfc000 task.ti: >> ffff880117dfc000 >> [ 63.980094] RIP: 0010:[] [] >> u32_destroy_key+0x27/0x6d >> [ 63.980094] RSP: 0018:ffff880117dffcc0 EFLAGS: 00010202 >> [ 63.980094] RAX: ffff880117dea690 RBX: ffff8800d02e0820 RCX: >> 0000000000000000 >> [ 63.980094] RDX: 0000000000000001 RSI: 0000000000000002 RDI: >> 6b6b6b6b6b6b6b6b >> [ 63.980094] RBP: ffff880117dffcd0 R08: 0000000000000000 R09: >> 0000000000000000 >> [ 63.980094] R10: 00006c0900006ba8 R11: 00006ba100006b9d R12: >> 0000000000000001 >> [ 63.980094] R13: ffff8800d02e0898 R14: ffffffff817e6d4d R15: >> ffff880117387a30 >> [ 63.980094] FS: 0000000000000000(0000) GS:ffff88011a800000(0000) >> knlGS:0000000000000000 >> [ 63.980094] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b >> [ 63.980094] CR2: 00007f07e6732fed CR3: 000000011665b000 CR4: >> 00000000000006e0 >> [ 63.980094] Stack: >> [ 63.980094] ffff88011a9cd300 ffffffff82051ac0 ffff880117dffce0 >> ffffffff817e6d68 >> [ 63.980094] ffff880117dffd70 ffffffff810cb4c7 ffffffff810cb3cd >> ffff880117dfffd8 >> [ 63.980094] ffff880117dea690 ffff880117dea690 ffff880117dfffd8 >> 000000000000000a >> [ 63.980094] Call Trace: >> [ 63.980094] [] u32_delete_key_freepf_rcu+0x1b/0x1d >> [ 63.980094] [] rcu_process_callbacks+0x3bb/0x691 >> [ 63.980094] [] ? rcu_process_callbacks+0x2c1/0x691 >> [ 63.980094] [] ? u32_destroy_key+0x6d/0x6d >> [ 63.980094] [] __do_softirq+0x142/0x323 >> [ 63.980094] [] run_ksoftirqd+0x23/0x53 >> [ 63.980094] [] smpboot_thread_fn+0x203/0x221 >> [ 63.980094] [] ? smpboot_unpark_thread+0x33/0x33 >> [ 63.980094] [] kthread+0xc9/0xd1 >> [ 63.980094] [] ? do_wait_for_common+0xf8/0x125 >> [ 63.980094] [] ? __kthread_parkme+0x61/0x61 >> [ 63.980094] [] ret_from_fork+0x7c/0xb0 >> [ 63.980094] [] ? __kthread_parkme+0x61/0x61 >> >> tp could be freed in call_rcu callback too, the order is not guaranteed. >> >> Cc: John Fastabend >> Signed-off-by: Cong Wang >> --- > > Thanks for catching this. What if we just drop tcf_exts_result > I can't see how its being used anymore. It appears to just be passed > around the ./net/sched files for some historic reason that is lost on > me. Would you mind testing a patch if I sent it out? > > Maybe Jamal can shed some light? > Sorry I should say its not needed to pass to the actions, tcf_exts_exec(). It _is_ needed here to get the class setup correct. And the tcf_exts_exec() stuff is a separate patch. Thanks again. Acked-by: John Fastabend > >> include/net/pkt_cls.h | 6 +----- >> net/sched/cls_u32.c | 10 ++++++---- >> 2 files changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >> index 73f9532..ef44ad9 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -20,11 +20,7 @@ int unregister_tcf_proto_ops(struct tcf_proto_ops >> *ops); >> static inline unsigned long >> __cls_set_class(unsigned long *clp, unsigned long cl) >> { >> - unsigned long old_cl; >> - >> - old_cl = *clp; >> - *clp = cl; >> - return old_cl; >> + return xchg(clp, cl); >> } >> >> static inline unsigned long >> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c >> index 4be3ebf..0472909 100644 >> --- a/net/sched/cls_u32.c >> +++ b/net/sched/cls_u32.c >> @@ -358,7 +358,6 @@ static int u32_destroy_key(struct tcf_proto *tp, >> struct tc_u_knode *n, >> bool free_pf) >> { >> - tcf_unbind_filter(tp, &n->res); >> tcf_exts_destroy(&n->exts); >> if (n->ht_down) >> n->ht_down->refcnt--; >> @@ -416,6 +415,7 @@ static int u32_delete_key(struct tcf_proto *tp, >> struct tc_u_knode *key) >> if (pkp == key) { >> RCU_INIT_POINTER(*kp, key->next); >> >> + tcf_unbind_filter(tp, &key->res); >> call_rcu(&key->rcu, u32_delete_key_freepf_rcu); >> return 0; >> } >> @@ -425,7 +425,7 @@ static int u32_delete_key(struct tcf_proto *tp, >> struct tc_u_knode *key) >> return 0; >> } >> >> -static void u32_clear_hnode(struct tc_u_hnode *ht) >> +static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht) >> { >> struct tc_u_knode *n; >> unsigned int h; >> @@ -434,6 +434,7 @@ static void u32_clear_hnode(struct tc_u_hnode *ht) >> while ((n = rtnl_dereference(ht->ht[h])) != NULL) { >> RCU_INIT_POINTER(ht->ht[h], >> rtnl_dereference(n->next)); >> + tcf_unbind_filter(tp, &n->res); >> call_rcu(&n->rcu, u32_delete_key_freepf_rcu); >> } >> } >> @@ -447,7 +448,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, >> struct tc_u_hnode *ht) >> >> WARN_ON(ht->refcnt); >> >> - u32_clear_hnode(ht); >> + u32_clear_hnode(tp, ht); >> >> hn = &tp_c->hlist; >> for (phn = rtnl_dereference(*hn); >> @@ -482,7 +483,7 @@ static void u32_destroy(struct tcf_proto *tp) >> ht; >> ht = rtnl_dereference(ht->next)) { >> ht->refcnt--; >> - u32_clear_hnode(ht); >> + u32_clear_hnode(tp, ht); >> } >> >> while ((ht = rtnl_dereference(tp_c->hlist)) != NULL) { >> @@ -731,6 +732,7 @@ static int u32_change(struct net *net, struct >> sk_buff *in_skb, >> } >> >> u32_replace_knode(tp, tp_c, new); >> + tcf_unbind_filter(tp, &n->res); >> call_rcu(&n->rcu, u32_delete_key_rcu); >> return 0; >> } >> > > -- John Fastabend Intel Corporation