[Patch net-next] net_sched: fix another crash in cls_tcindex

[Patch net-next] net_sched: fix another crash in cls_tcindex

[Cong Wang]

This patch fixes the following crash:

[  166.670795] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  166.674230] IP: [<ffffffff814b739f>] __list_del_entry+0x5c/0x98
[  166.674230] PGD d0ea5067 PUD ce7fc067 PMD 0
[  166.674230] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  166.674230] CPU: 1 PID: 775 Comm: tc Not tainted 3.17.0-rc6+ #642
[  166.674230] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  166.674230] task: ffff8800d03c4d20 ti: ffff8800cae7c000 task.ti: ffff8800cae7c000
[  166.674230] RIP: 0010:[<ffffffff814b739f>]  [<ffffffff814b739f>] __list_del_entry+0x5c/0x98
[  166.674230] RSP: 0018:ffff8800cae7f7d0  EFLAGS: 00010207
[  166.674230] RAX: 0000000000000000 RBX: ffff8800cba8d700 RCX: ffff8800cba8d700
[  166.674230] RDX: 0000000000000000 RSI: dead000000200200 RDI: ffff8800cba8d700
[  166.674230] RBP: ffff8800cae7f7d0 R08: 0000000000000001 R09: 0000000000000001
[  166.674230] R10: 0000000000000000 R11: 000000000000859a R12: ffffffffffffffe8
[  166.674230] R13: ffff8800cba8c5b8 R14: 0000000000000001 R15: ffff8800cba8d700
[  166.674230] FS:  00007fdb5f04a740(0000) GS:ffff88011a800000(0000) knlGS:0000000000000000
[  166.674230] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  166.674230] CR2: 0000000000000000 CR3: 00000000cf929000 CR4: 00000000000006e0
[  166.674230] Stack:
[  166.674230]  ffff8800cae7f7e8 ffffffff814b73e8 ffff8800cba8d6e8 ffff8800cae7f828
[  166.674230]  ffffffff817caeec 0000000000000046 ffff8800cba8c5b0 ffff8800cba8c5b8
[  166.674230]  0000000000000000 0000000000000001 ffff8800cf8e33e8 ffff8800cae7f848
[  166.674230] Call Trace:
[  166.674230]  [<ffffffff814b73e8>] list_del+0xd/0x2b
[  166.674230]  [<ffffffff817caeec>] tcf_action_destroy+0x4c/0x71
[  166.674230]  [<ffffffff817ca0ce>] tcf_exts_destroy+0x20/0x2d
[  166.674230]  [<ffffffff817ec2b5>] tcindex_delete+0x196/0x1b7

struct list_head can not be simply copied and we should always init it.

Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 8d0e83d..30f10fb 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -254,10 +254,15 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	cp->tp = tp;
 
 	if (p->perfect) {
+		int i;
+
 		cp->perfect = kmemdup(p->perfect,
 				      sizeof(*r) * cp->hash, GFP_KERNEL);
 		if (!cp->perfect)
 			goto errout;
+		for (i = 0; i < cp->hash; i++)
+			tcf_exts_init(&cp->perfect[i].exts,
+				      TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 		balloc = 1;
 	}
 	cp->h = p->h;
@@ -353,6 +358,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 		f = kzalloc(sizeof(*f), GFP_KERNEL);
 		if (!f)
 			goto errout_alloc;
+		f->key = handle;
+		tcindex_filter_result_init(&f->result);
+		f->next = NULL;
 	}
 
 	if (tb[TCA_TCINDEX_CLASSID]) {
@@ -376,9 +384,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 		struct tcindex_filter *nfp;
 		struct tcindex_filter __rcu **fp;
 
-		f->key = handle;
-		f->result = new_filter_result;
-		f->next = NULL;
+		tcf_exts_change(tp, &f->result.exts, &r->exts);
 
 		fp = cp->h + (handle % cp->hash);
 		for (nfp = rtnl_dereference(*fp);
-- 
1.8.3.1

[Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[Cong Wang]

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>
---
 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;
 	}
-- 
1.8.3.1

Re: [Patch net-next] net_sched: fix another crash in cls_tcindex

[John Fastabend]

On 09/30/2014 04:07 PM, Cong Wang wrote:
> This patch fixes the following crash:
>
> [  166.670795] BUG: unable to handle kernel NULL pointer dereference at           (null)
> [  166.674230] IP: [<ffffffff814b739f>] __list_del_entry+0x5c/0x98
> [  166.674230] PGD d0ea5067 PUD ce7fc067 PMD 0
> [  166.674230] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> [  166.674230] CPU: 1 PID: 775 Comm: tc Not tainted 3.17.0-rc6+ #642
> [  166.674230] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [  166.674230] task: ffff8800d03c4d20 ti: ffff8800cae7c000 task.ti: ffff8800cae7c000
> [  166.674230] RIP: 0010:[<ffffffff814b739f>]  [<ffffffff814b739f>] __list_del_entry+0x5c/0x98
> [  166.674230] RSP: 0018:ffff8800cae7f7d0  EFLAGS: 00010207
> [  166.674230] RAX: 0000000000000000 RBX: ffff8800cba8d700 RCX: ffff8800cba8d700
> [  166.674230] RDX: 0000000000000000 RSI: dead000000200200 RDI: ffff8800cba8d700
> [  166.674230] RBP: ffff8800cae7f7d0 R08: 0000000000000001 R09: 0000000000000001
> [  166.674230] R10: 0000000000000000 R11: 000000000000859a R12: ffffffffffffffe8
> [  166.674230] R13: ffff8800cba8c5b8 R14: 0000000000000001 R15: ffff8800cba8d700
> [  166.674230] FS:  00007fdb5f04a740(0000) GS:ffff88011a800000(0000) knlGS:0000000000000000
> [  166.674230] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  166.674230] CR2: 0000000000000000 CR3: 00000000cf929000 CR4: 00000000000006e0
> [  166.674230] Stack:
> [  166.674230]  ffff8800cae7f7e8 ffffffff814b73e8 ffff8800cba8d6e8 ffff8800cae7f828
> [  166.674230]  ffffffff817caeec 0000000000000046 ffff8800cba8c5b0 ffff8800cba8c5b8
> [  166.674230]  0000000000000000 0000000000000001 ffff8800cf8e33e8 ffff8800cae7f848
> [  166.674230] Call Trace:
> [  166.674230]  [<ffffffff814b73e8>] list_del+0xd/0x2b
> [  166.674230]  [<ffffffff817caeec>] tcf_action_destroy+0x4c/0x71
> [  166.674230]  [<ffffffff817ca0ce>] tcf_exts_destroy+0x20/0x2d
> [  166.674230]  [<ffffffff817ec2b5>] tcindex_delete+0x196/0x1b7
>
> struct list_head can not be simply copied and we should always init it.
>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---

Thanks again.

Acked-by: John Fastabend <john.r.fastabend@intel.com>


-- 
John Fastabend         Intel Corporation

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[John Fastabend]

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?


>   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

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[John Fastabend]

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

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[John Fastabend]

On 09/30/2014 06:18 PM, John Fastabend wrote:
> 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>
> 
> 
>>

Its worth noting why this is safe. Any running schedulers will either
read the valid class field or it will be zeroed.

All schedulers today when the class is 0 do a lookup using the
same call used by the tcf_exts_bind(). So even if we have a running
classifier hit the null class pointer it will do a lookup and get
to the same result. This is particularly fragile at the moment because
the only way to verify this is to audit the schedulers call sites.

I think we need a helper to ensure the code doesn't get broken in
a subtle way in the future. At very least it should be documented.
I'll try to draft a follow up patch to use a helper routine for
this and document it.

Similar patches are needed for basic, fw, route, rsvp, and tcindex.

.John

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[Cong Wang]

On Wed, Oct 1, 2014 at 1:50 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
>
> Its worth noting why this is safe. Any running schedulers will either
> read the valid class field or it will be zeroed.
>
> All schedulers today when the class is 0 do a lookup using the
> same call used by the tcf_exts_bind(). So even if we have a running
> classifier hit the null class pointer it will do a lookup and get
> to the same result. This is particularly fragile at the moment because
> the only way to verify this is to audit the schedulers call sites.

Yeah, maybe David can squash this into changelog.

>
> I think we need a helper to ensure the code doesn't get broken in
> a subtle way in the future. At very least it should be documented.
> I'll try to draft a follow up patch to use a helper routine for
> this and document it.
>
> Similar patches are needed for basic, fw, route, rsvp, and tcindex.
>

Please do fix them. Meanwhile, I am trying to refactor these pieces
of ugly code.

Thanks.

Re: [Patch net-next] net_sched: fix another crash in cls_tcindex

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 30 Sep 2014 16:07:23 -0700

> This patch fixes the following crash:
 ...
> struct list_head can not be simply copied and we should always init it.
> 
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 30 Sep 2014 16:07:24 -0700

> This fixes the following crash:
 ...
> 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>

Applied, and I added John's description of why this is legal to the
commit message.

Re: [Patch net-next] net_sched: avoid calling tcf_unbind_filter() in call_rcu callback

[John Fastabend]

On 10/01/2014 07:01 PM, David Miller wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 30 Sep 2014 16:07:24 -0700
>
>> This fixes the following crash:
>   ...
>> 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>
>
> Applied, and I added John's description of why this is legal to the
> commit message.
> --

Thanks, I'll have another series shortly to fix the
other classifiers with the same issue and pull 'tp'
out of the ematch stuff.

Passing around free'd pointers that never get used
through the ematch code wont cause a crash but there is
no reason to propagate it like this.

.John

-- 
John Fastabend         Intel Corporation