From: John Fastabend <john.fastabend@gmail.com>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net,
John Fastabend <john.r.fastabend@intel.com>
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 [thread overview]
Message-ID: <542B5679.3060601@gmail.com> (raw)
In-Reply-To: <542B5096.2040106@gmail.com>
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:[<ffffffff817e6d07>] [<ffffffff817e6d07>]
>> 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] [<ffffffff817e6d68>] u32_delete_key_freepf_rcu+0x1b/0x1d
>> [ 63.980094] [<ffffffff810cb4c7>] rcu_process_callbacks+0x3bb/0x691
>> [ 63.980094] [<ffffffff810cb3cd>] ? rcu_process_callbacks+0x2c1/0x691
>> [ 63.980094] [<ffffffff817e6d4d>] ? u32_destroy_key+0x6d/0x6d
>> [ 63.980094] [<ffffffff810780a4>] __do_softirq+0x142/0x323
>> [ 63.980094] [<ffffffff810782a8>] run_ksoftirqd+0x23/0x53
>> [ 63.980094] [<ffffffff81092126>] smpboot_thread_fn+0x203/0x221
>> [ 63.980094] [<ffffffff81091f23>] ? smpboot_unpark_thread+0x33/0x33
>> [ 63.980094] [<ffffffff8108e44d>] kthread+0xc9/0xd1
>> [ 63.980094] [<ffffffff819e00ea>] ? do_wait_for_common+0xf8/0x125
>> [ 63.980094] [<ffffffff8108e384>] ? __kthread_parkme+0x61/0x61
>> [ 63.980094] [<ffffffff819e43ec>] ret_from_fork+0x7c/0xb0
>> [ 63.980094] [<ffffffff8108e384>] ? __kthread_parkme+0x61/0x61
>>
>> tp could be freed in call_rcu callback too, the order is not guaranteed.
>>
>> Cc: John Fastabend <john.r.fastabend@intel.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>
> 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 <john.r.fastabend@intel.com>
>
>> 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
next prev parent reply other threads:[~2014-10-01 1:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-30 23:07 [Patch net-next] net_sched: fix another crash in cls_tcindex Cong Wang
2014-09-30 23:07 ` [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback Cong Wang
2014-10-01 0:53 ` John Fastabend
2014-10-01 1:18 ` John Fastabend [this message]
2014-10-01 20:50 ` John Fastabend
2014-10-01 23:36 ` Cong Wang
2014-10-02 2:01 ` David Miller
2014-10-02 18:04 ` John Fastabend
2014-09-30 23:50 ` [Patch net-next] net_sched: fix another crash in cls_tcindex John Fastabend
2014-10-02 2:01 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=542B5679.3060601@gmail.com \
--to=john.fastabend@gmail.com \
--cc=davem@davemloft.net \
--cc=john.r.fastabend@intel.com \
--cc=netdev@vger.kernel.org \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).