[Patch net-next] net_sched: fix a regression in tc actions

[Patch net-next] net_sched: fix a regression in tc actions

[Cong Wang]

This patch fixes:
1) pass mask rather than size to tcf_hashinfo_init()
2) the cleanup should be in reversed order in mirred_cleanup_module()

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: 369ba56787d7469c0afd ("net_sched: init struct tcf_hashinfo at register time")
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

---
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 5d350c5..9cc6717 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -586,7 +586,7 @@ MODULE_LICENSE("GPL");
 
 static int __init csum_init_module(void)
 {
-	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK);
 	if (err)
 		return err;
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 1e6e0e7..dea9273 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -208,7 +208,7 @@ MODULE_LICENSE("GPL");
 
 static int __init gact_init_module(void)
 {
-	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK);
 	if (err)
 		return err;
 #ifdef CONFIG_GACT_PROB
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 8344380..e13ecbb 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -314,7 +314,7 @@ MODULE_ALIAS("act_xt");
 static int __init ipt_init_module(void)
 {
 	int ret1, ret2, err;
-	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK+1);
+	err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK);
 	if (err)
 		return err;
 
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 199fc98..9dbb8cd 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -276,7 +276,7 @@ static int __init mirred_init_module(void)
 	if (err)
 		return err;
 
-	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK+1);
+	err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK);
 	if (err) {
 		unregister_netdevice_notifier(&mirred_device_notifier);
 		return err;
@@ -287,9 +287,9 @@ static int __init mirred_init_module(void)
 
 static void __exit mirred_cleanup_module(void)
 {
-	unregister_netdevice_notifier(&mirred_device_notifier);
-	tcf_hashinfo_destroy(&mirred_hash_info);
 	tcf_unregister_action(&act_mirred_ops);
+	tcf_hashinfo_destroy(&mirred_hash_info);
+	unregister_netdevice_notifier(&mirred_device_notifier);
 }
 
 module_init(mirred_init_module);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 409fe71..921fea4 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -310,7 +310,7 @@ MODULE_LICENSE("GPL");
 
 static int __init nat_init_module(void)
 {
-	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK);
 	if (err)
 		return err;
 	return tcf_register_action(&act_nat_ops);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index aa5347c..e2520e9 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -246,7 +246,7 @@ MODULE_LICENSE("GPL");
 
 static int __init pedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK);
 	if (err)
 		return err;
 	return tcf_register_action(&act_pedit_ops);
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 7b23ab0..819a9a4 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -396,7 +396,7 @@ static struct tc_action_ops act_police_ops = {
 static int __init
 police_init_module(void)
 {
-	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK);
 	if (err)
 		return err;
 	err = tcf_register_action(&act_police_ops);
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 2d7a0eb..81aebc1 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -203,7 +203,7 @@ MODULE_LICENSE("GPL");
 static int __init simp_init_module(void)
 {
 	int err, ret;
-	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK+1);
+	err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK);
 	if (err)
 		return err;
 
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 90ed04a..aa0a4c0 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -203,7 +203,7 @@ MODULE_LICENSE("GPL");
 
 static int __init skbedit_init_module(void)
 {
-	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK+1);
+	int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK);
 	if (err)
 		return err;
 	return tcf_register_action(&act_skbedit_ops);

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Jamal Hadi Salim]

On 12/20/13 03:08, Cong Wang wrote:
> This patch fixes:
> 1) pass mask rather than size to tcf_hashinfo_init()
> 2) the cleanup should be in reversed order in mirred_cleanup_module()
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: 369ba56787d7469c0afd ("net_sched: init struct tcf_hashinfo at register time")
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>

Please Cc me - my filters will catch it faster.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>


cheers,
jamal

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Eric Dumazet]

On Fri, 2013-12-20 at 07:15 -0500, Jamal Hadi Salim wrote:
> On 12/20/13 03:08, Cong Wang wrote:
> > This patch fixes:
> > 1) pass mask rather than size to tcf_hashinfo_init()
> > 2) the cleanup should be in reversed order in mirred_cleanup_module()
> >
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Fixes: 369ba56787d7469c0afd ("net_sched: init struct tcf_hashinfo at register time")
> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> 
> Please Cc me - my filters will catch it faster.
> 
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

This doesn't fix the issue, as I said earlier

Even not using a module but static linking for act_mirred triggers the
following :

[   86.909685] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)^M
[   86.917183] BUG: unable to handle kernel paging request at ffffffff818a4b80^M
[   86.924190] IP: [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   86.929439] PGD 180c067 PUD 180d063 PMD 202e851063 PTE 80000000018a4163^M
[   86.936144] Oops: 0011 [#1] SMP ^M
[   86.939405] Modules linked in: cls_tcindex sch_dsmark xt_multiport iptable_mangle w1_therm wire cdc_acm uhci_hcd ehci_hcd i2c_dev i2c_i801 i2c_core msr cpuid ib_uverbs mlx4_ib ib_sa ib_mad mlx4_en ib_core mlx4_core ipv6^M
[   86.959307] CPU: 11 PID: 11378 Comm: BweTCTree_DoWor Not tainted 3.13.0-smp-DEV #413^M
[   86.973964] task: ffff880078fc6690 ti: ffff882016886000 task.ti: ffff882016886000^M
[   86.981467] RIP: 0010:[<ffffffff818a4b80>]  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   86.989153] RSP: 0018:ffff8820168879b0  EFLAGS: 00010206^M
[   86.994482] RAX: 0000000000010000 RBX: ffff881015c59480 RCX: 0000000000000000^M
[   87.001635] RDX: ffffffff818a4b50 RSI: ffffffffa0059790 RDI: ffff881015c59480^M
[   87.008772] RBP: ffff882016887aa8 R08: ffff88107f4004c0 R09: ffff881015c59480^M
[   87.015922] R10: ffff880000077680 R11: 00000000ffffff97 R12: ffff881016ae9160^M
[   87.023094] R13: ffff881016ae9000 R14: ffff881016ae9000 R15: ffff88102e405800^M
[   87.030277] FS:  00007f29f3858700(0000) GS:ffff88107fb60000(0000) knlGS:0000000000000000^M
[   87.038386] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033^M
[   87.044146] CR2: ffffffff818a4b80 CR3: 0000002030af1000 CR4: 00000000001407e0^M
[   87.051301] Stack:^M
[   87.053314]  ffffffff814dd0e1 0000000000000028 00000000ffffffff ffffffff818a4b50^M
[   87.060774]  ffff880000077680 ffff88102e405824 0000000800000000 ffffffff818a1940^M
[   87.068232]  0000000000000000 ffffffff8169ca80 0001000000010000 ffff882016887a68^M
[   87.075750] Call Trace:^M
[   87.078202]  [<ffffffff814dd0e1>] ? tc_ctl_tfilter+0x4a1/0x760^M
[   87.084071]  [<ffffffff814e6fb6>] ? __netlink_sendskb+0x56/0x130^M
[   87.090108]  [<ffffffff814caa04>] rtnetlink_rcv_msg+0xa4/0x240^M
[   87.095945]  [<ffffffff814ca960>] ? __rtnl_unlock+0x20/0x20^M
[   87.101538]  [<ffffffff814e9e91>] netlink_rcv_skb+0xb1/0xc0^M
[   87.107118]  [<ffffffff814c7635>] rtnetlink_rcv+0x25/0x40^M
[   87.112528]  [<ffffffff814e973d>] netlink_unicast+0x14d/0x1f0^M
[   87.118314]  [<ffffffff814e9ae3>] netlink_sendmsg+0x303/0x410^M
[   87.124092]  [<ffffffff814a048c>] sock_sendmsg+0x9c/0xd0^M
[   87.129451]  [<ffffffff814a0da0>] ? move_addr_to_kernel+0x40/0xa0^M
[   87.135566]  [<ffffffff814add21>] ? verify_iovec+0x51/0xd0^M
[   87.141064]  [<ffffffff814a0c21>] ___sys_sendmsg+0x3c1/0x3d0^M
[   87.146747]  [<ffffffff815690bc>] ? __do_page_fault+0x26c/0x500^M
[   87.152689]  [<ffffffff811821ab>] ? do_brk+0x1bb/0x320^M
[   87.157830]  [<ffffffff814a1809>] __sys_sendmsg+0x49/0x90^M
[   87.163250]  [<ffffffff814a1862>] SyS_sendmsg+0x12/0x20^M
[   87.168484]  [<ffffffff8156d992>] system_call_fastpath+0x16/0x1b^M
[   87.174518] Code: ff ff ff 80 97 05 a0 ff ff ff ff 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 80 4b 8a 81 ff ff ff ff 80 4b 8a 81 ff ff ff ff <70> 4b 8a 81 ff ff ff ff 70 4b 8a 81 ff ff ff ff c0 9d ba 81 ff ^M
[   87.194642] RIP  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   87.199983]  RSP <ffff8820168879b0>^M
[   87.203472] CR2: ffffffff818a4b80^M
[   87.206785] ---[ end trace 8ef37a4cc4cb5a8c ]---^M
[   87.211408] Kernel panic - not syncing: Fatal exception^M
[   87.216763] Rebooting in 10 seconds..^M

We actually try to execute code at ffffffff818a4b80 == &act_mirred_ops

ffffffff818a4b40 d cls_mod_lock
ffffffff818a4b50 d tcf_proto_base
ffffffff818a4b60 d act_mod_lock
ffffffff818a4b70 d act_base
ffffffff818a4b80 d act_mirred_ops
ffffffff818a4bf0 d mirred_device_notifier
ffffffff818a4c10 d mirred_list
ffffffff818a4c20 d _rs.42828
ffffffff818a4c40 d ematch_mod_lock
ffffffff818a4c50 d ematch_ops

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Jamal Hadi Salim]

On 12/20/13 09:37, Eric Dumazet wrote:
> On Fri, 2013-12-20 at 07:15 -0500, Jamal Hadi Salim wrote:

>> Please Cc me - my filters will catch it faster.
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> This doesn't fix the issue, as I said earlier
>

That is strange - I have net-next from when these patches made it
in and no problem booting with statically linked mirred.
I will try to pull the latest later and rebuild.
The patch above will fix the issue on the dumping of actions. I am only
these additional patches I havent paid much attention to.

cheers,
jamal


> Even not using a module but static linking for act_mirred triggers the
> following :
>
> [   86.909685] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)^M
> [   86.917183] BUG: unable to handle kernel paging request at ffffffff818a4b80^M
> [   86.924190] IP: [<ffffffff818a4b80>] 0xffffffff818a4b80^M
> [   86.929439] PGD 180c067 PUD 180d063 PMD 202e851063 PTE 80000000018a4163^M
> [   86.936144] Oops: 0011 [#1] SMP ^M
> [   86.939405] Modules linked in: cls_tcindex sch_dsmark xt_multiport iptable_mangle w1_therm wire cdc_acm uhci_hcd ehci_hcd i2c_dev i2c_i801 i2c_core msr cpuid ib_uverbs mlx4_ib ib_sa ib_mad mlx4_en ib_core mlx4_core ipv6^M
> [   86.959307] CPU: 11 PID: 11378 Comm: BweTCTree_DoWor Not tainted 3.13.0-smp-DEV #413^M
> [   86.973964] task: ffff880078fc6690 ti: ffff882016886000 task.ti: ffff882016886000^M
> [   86.981467] RIP: 0010:[<ffffffff818a4b80>]  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
> [   86.989153] RSP: 0018:ffff8820168879b0  EFLAGS: 00010206^M
> [   86.994482] RAX: 0000000000010000 RBX: ffff881015c59480 RCX: 0000000000000000^M
> [   87.001635] RDX: ffffffff818a4b50 RSI: ffffffffa0059790 RDI: ffff881015c59480^M
> [   87.008772] RBP: ffff882016887aa8 R08: ffff88107f4004c0 R09: ffff881015c59480^M
> [   87.015922] R10: ffff880000077680 R11: 00000000ffffff97 R12: ffff881016ae9160^M
> [   87.023094] R13: ffff881016ae9000 R14: ffff881016ae9000 R15: ffff88102e405800^M
> [   87.030277] FS:  00007f29f3858700(0000) GS:ffff88107fb60000(0000) knlGS:0000000000000000^M
> [   87.038386] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033^M
> [   87.044146] CR2: ffffffff818a4b80 CR3: 0000002030af1000 CR4: 00000000001407e0^M
> [   87.051301] Stack:^M
> [   87.053314]  ffffffff814dd0e1 0000000000000028 00000000ffffffff ffffffff818a4b50^M
> [   87.060774]  ffff880000077680 ffff88102e405824 0000000800000000 ffffffff818a1940^M
> [   87.068232]  0000000000000000 ffffffff8169ca80 0001000000010000 ffff882016887a68^M
> [   87.075750] Call Trace:^M
> [   87.078202]  [<ffffffff814dd0e1>] ? tc_ctl_tfilter+0x4a1/0x760^M
> [   87.084071]  [<ffffffff814e6fb6>] ? __netlink_sendskb+0x56/0x130^M
> [   87.090108]  [<ffffffff814caa04>] rtnetlink_rcv_msg+0xa4/0x240^M
> [   87.095945]  [<ffffffff814ca960>] ? __rtnl_unlock+0x20/0x20^M
> [   87.101538]  [<ffffffff814e9e91>] netlink_rcv_skb+0xb1/0xc0^M
> [   87.107118]  [<ffffffff814c7635>] rtnetlink_rcv+0x25/0x40^M
> [   87.112528]  [<ffffffff814e973d>] netlink_unicast+0x14d/0x1f0^M
> [   87.118314]  [<ffffffff814e9ae3>] netlink_sendmsg+0x303/0x410^M
> [   87.124092]  [<ffffffff814a048c>] sock_sendmsg+0x9c/0xd0^M
> [   87.129451]  [<ffffffff814a0da0>] ? move_addr_to_kernel+0x40/0xa0^M
> [   87.135566]  [<ffffffff814add21>] ? verify_iovec+0x51/0xd0^M
> [   87.141064]  [<ffffffff814a0c21>] ___sys_sendmsg+0x3c1/0x3d0^M
> [   87.146747]  [<ffffffff815690bc>] ? __do_page_fault+0x26c/0x500^M
> [   87.152689]  [<ffffffff811821ab>] ? do_brk+0x1bb/0x320^M
> [   87.157830]  [<ffffffff814a1809>] __sys_sendmsg+0x49/0x90^M
> [   87.163250]  [<ffffffff814a1862>] SyS_sendmsg+0x12/0x20^M
> [   87.168484]  [<ffffffff8156d992>] system_call_fastpath+0x16/0x1b^M
> [   87.174518] Code: ff ff ff 80 97 05 a0 ff ff ff ff 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 80 4b 8a 81 ff ff ff ff 80 4b 8a 81 ff ff ff ff <70> 4b 8a 81 ff ff ff ff 70 4b 8a 81 ff ff ff ff c0 9d ba 81 ff ^M
> [   87.194642] RIP  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
> [   87.199983]  RSP <ffff8820168879b0>^M
> [   87.203472] CR2: ffffffff818a4b80^M
> [   87.206785] ---[ end trace 8ef37a4cc4cb5a8c ]---^M
> [   87.211408] Kernel panic - not syncing: Fatal exception^M
> [   87.216763] Rebooting in 10 seconds..^M
>
> We actually try to execute code at ffffffff818a4b80 == &act_mirred_ops
>
> ffffffff818a4b40 d cls_mod_lock
> ffffffff818a4b50 d tcf_proto_base
> ffffffff818a4b60 d act_mod_lock
> ffffffff818a4b70 d act_base
> ffffffff818a4b80 d act_mirred_ops
> ffffffff818a4bf0 d mirred_device_notifier
> ffffffff818a4c10 d mirred_list
> ffffffff818a4c20 d _rs.42828
> ffffffff818a4c40 d ematch_mod_lock
> ffffffff818a4c50 d ematch_ops
>
>

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Eric Dumazet]

On Fri, 2013-12-20 at 09:46 -0500, Jamal Hadi Salim wrote:

> That is strange - I have net-next from when these patches made it
> in and no problem booting with statically linked mirred.
> I will try to pull the latest later and rebuild.
> The patch above will fix the issue on the dumping of actions. I am only
> these additional patches I havent paid much attention to.

Please note boot is OK.

Some application plays netlink games, I have no idea which ones.

Not sure I'll have the time to take a deeper look in following days.

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Eric Dumazet]

On Fri, 2013-12-20 at 08:07 -0800, Eric Dumazet wrote:
> On Fri, 2013-12-20 at 09:46 -0500, Jamal Hadi Salim wrote:
> 
> > That is strange - I have net-next from when these patches made it
> > in and no problem booting with statically linked mirred.
> > I will try to pull the latest later and rebuild.
> > The patch above will fix the issue on the dumping of actions. I am only
> > these additional patches I havent paid much attention to.
> 
> Please note boot is OK.
> 
> Some application plays netlink games, I have no idea which ones.
> 
> Not sure I'll have the time to take a deeper look in following days.
> 
> 

The winner is :

edumazet@manihi:~/git/net-next$ git bisect log
# bad: [79c11f2e3fd3e0e3594d4155821ef426153a773f] sch_cbq: remove
unnecessary null pointer check
# good: [f52ed89971adbe79b6438c459814034707b8ab91] pkt_sched: fq: fix
pacing for small frames
git bisect start '79c11f2e3fd3' 'f52ed89971ad' '--' 'net/sched'
# good: [4f8f61eb4341be07b4a8d046f646fcf934a89949] net_sched: expand
control flow of macro SKIP_NONLOCAL
git bisect good 4f8f61eb4341be07b4a8d046f646fcf934a89949
# good: [89819dc01f4c5920783f561597a48d9d75220e9e] net_sched: convert
tcf_hashinfo to hlist and use spinlock
git bisect good 89819dc01f4c5920783f561597a48d9d75220e9e
# good: [d55d282e6af88120ad90e93a88f70e3116dc0e3d] sch_tbf: use do_div()
for 64-bit divide
git bisect good d55d282e6af88120ad90e93a88f70e3116dc0e3d
# bad: [143c9054949436cb05e468439dc5e46231f33d09] Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
git bisect bad 143c9054949436cb05e468439dc5e46231f33d09
# bad: [3627287463b4acddb83d24fabb1e0a304e39565c] net_sched: convert
tcf_proto_ops to use struct list_head
git bisect bad 3627287463b4acddb83d24fabb1e0a304e39565c
# good: [1f747c26c48bb290c79c34e155860c7e2ec3926a] net_sched: convert
tc_action_ops to use struct list_head
git bisect good 1f747c26c48bb290c79c34e155860c7e2ec3926a
# first bad commit: [3627287463b4acddb83d24fabb1e0a304e39565c]
net_sched: convert tcf_proto_ops to use struct list_head

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Eric Dumazet]

On Fri, 2013-12-20 at 09:20 -0800, Eric Dumazet wrote:

> 
> The winner is :
> 

> # first bad commit: [3627287463b4acddb83d24fabb1e0a304e39565c]
> net_sched: convert tcf_proto_ops to use struct list_head
> 

I am testing a fix.

[PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

list_for_each_entry(t, &tcf_proto_base, head) doesn't
exit with t = NULL if we reached the end of the list.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 3627287463b4 ("net_sched: convert tcf_proto_ops to use struct 
list_head")
Cc: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_api.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6b085cf27a65..12e882ef596b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -40,20 +40,20 @@ static DEFINE_RWLOCK(cls_mod_lock);
 
 static const struct tcf_proto_ops *tcf_proto_lookup_ops(struct nlattr *kind)
 {
-	const struct tcf_proto_ops *t = NULL;
+	const struct tcf_proto_ops *t, *res = NULL;
 
 	if (kind) {
 		read_lock(&cls_mod_lock);
 		list_for_each_entry(t, &tcf_proto_base, head) {
 			if (nla_strcmp(kind, t->kind) == 0) {
-				if (!try_module_get(t->owner))
-					t = NULL;
+				if (try_module_get(t->owner))
+					res = t;
 				break;
 			}
 		}
 		read_unlock(&cls_mod_lock);
 	}
-	return t;
+	return res;
 }
 
 /* Register(unregister) new classifier type */
@@ -82,15 +82,13 @@ int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
 	int rc = -ENOENT;
 
 	write_lock(&cls_mod_lock);
-	list_for_each_entry(t, &tcf_proto_base, head)
-		if (t == ops)
+	list_for_each_entry(t, &tcf_proto_base, head) {
+		if (t == ops) {
+			list_del(&t->head);
+			rc = 0;
 			break;
-
-	if (!t)
-		goto out;
-	list_del(&t->head);
-	rc = 0;
-out:
+		}
+	}
 	write_unlock(&cls_mod_lock);
 	return rc;
 }

Re: [Patch net-next] net_sched: fix a regression in tc actions

[Cong Wang]

On Fri, Dec 20, 2013 at 6:37 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-12-20 at 07:15 -0500, Jamal Hadi Salim wrote:
>> On 12/20/13 03:08, Cong Wang wrote:
>> > This patch fixes:
>> > 1) pass mask rather than size to tcf_hashinfo_init()
>> > 2) the cleanup should be in reversed order in mirred_cleanup_module()
>> >
>> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
>> > Fixes: 369ba56787d7469c0afd ("net_sched: init struct tcf_hashinfo at register time")
>> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> >
>>
>> Please Cc me - my filters will catch it faster.
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> This doesn't fix the issue, as I said earlier


When did I say I fixed it? :) Below is what I said:

> I just sent a fix for the problem I see, not sure it fixes
> the bug you reported too.
>
> I will test it as a module tomorrow, if you don't send a fix
> before that.

Thanks.

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Cong Wang]

On Fri, Dec 20, 2013 at 10:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> list_for_each_entry(t, &tcf_proto_base, head) doesn't
> exit with t = NULL if we reached the end of the list.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 3627287463b4 ("net_sched: convert tcf_proto_ops to use struct
> list_head")
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/cls_api.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

The bug you reported to me yesterday is in mirred action module cleanup,
so how is it possible to be fixed in cls_api.c? It should be act_api.c.

If you meant to fix a different bug, please include the backtrace in the log?

Your patch looks good generally, but actually I doubt we need to check for
an existing ops when unregistering, since we almost for sure only unregister
existing ones as they are all static allocated.

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Eric Dumazet]

On Fri, 2013-12-20 at 11:11 -0800, Cong Wang wrote:
> On Fri, Dec 20, 2013 at 10:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > list_for_each_entry(t, &tcf_proto_base, head) doesn't
> > exit with t = NULL if we reached the end of the list.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: 3627287463b4 ("net_sched: convert tcf_proto_ops to use struct
> > list_head")
> > Cc: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  net/sched/cls_api.c |   22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> The bug you reported to me yesterday is in mirred action module cleanup,
> so how is it possible to be fixed in cls_api.c? It should be act_api.c.
> 
> If you meant to fix a different bug, please include the backtrace in the log?

Why ? 

I think the bug is obvious and do not need a verbose stack trace in the
changelog.

Even you were not able to decrypt it, as you thought it was a bug in
mirred code. You keep saying it was cause by module cleanup, while
I already said the module was not removed at all.

> 
> Your patch looks good generally, but actually I doubt we need to check for
> an existing ops when unregistering, since we almost for sure only unregister
> existing ones as they are all static allocated.

Well, lets fix the bug first. I do not think there is any point trying
to be smart in this slow path. Particularly if I have to spend 2 more
hours to fix some random bug.

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Cong Wang]

On Fri, Dec 20, 2013 at 11:36 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-12-20 at 11:11 -0800, Cong Wang wrote:
>> On Fri, Dec 20, 2013 at 10:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > From: Eric Dumazet <edumazet@google.com>
>> >
>> > list_for_each_entry(t, &tcf_proto_base, head) doesn't
>> > exit with t = NULL if we reached the end of the list.
>> >
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>> > Fixes: 3627287463b4 ("net_sched: convert tcf_proto_ops to use struct
>> > list_head")
>> > Cc: Cong Wang <xiyou.wangcong@gmail.com>
>> > ---
>> >  net/sched/cls_api.c |   22 ++++++++++------------
>> >  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> The bug you reported to me yesterday is in mirred action module cleanup,
>> so how is it possible to be fixed in cls_api.c? It should be act_api.c.
>>
>> If you meant to fix a different bug, please include the backtrace in the log?
>
> Why ?
>
> I think the bug is obvious and do not need a verbose stack trace in the
> changelog.
>
> Even you were not able to decrypt it, as you thought it was a bug in
> mirred code. You keep saying it was cause by module cleanup, while
> I already said the module was not removed at all.

Then why mirred_cleanup_module() appears in the top of your backtrace?

>
>>
>> Your patch looks good generally, but actually I doubt we need to check for
>> an existing ops when unregistering, since we almost for sure only unregister
>> existing ones as they are all static allocated.
>
> Well, lets fix the bug first. I do not think there is any point trying
> to be smart in this slow path. Particularly if I have to spend 2 more
> hours to fix some random bug.
>
>

The current code is smarter actually. Sure, please fix act_api.c as well?

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Eric Dumazet]

On Fri, 2013-12-20 at 11:57 -0800, Cong Wang wrote:

> Then why mirred_cleanup_module() appears in the top of your backtrace?

Because you have not understood the trace ?

Thats exactly why its not useful at all.


[   76.656061] RIP: 0010:[<ffffffffa008d882>]  [<ffffffffa008d882>] mirred_cleanup_module+0x13a/0x2a [act_mirred]^M


This means RIP is _after_ mirred_cleanup_module() body, as this function size if 0x2a bytes

We were trying to execute a data portion which happens to be in
the neighboring of innocent mirred_cleanup_module()

When not using module, trace was different and made no sense anyway :

[   86.909685] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)^M
[   86.917183] BUG: unable to handle kernel paging request at ffffffff818a4b80^M
[   86.924190] IP: [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   86.929439] PGD 180c067 PUD 180d063 PMD 202e851063 PTE 80000000018a4163^M
[   86.936144] Oops: 0011 [#1] SMP ^M
[   86.939405] Modules linked in: cls_tcindex sch_dsmark xt_multiport iptable_mangle w1_therm wire cdc_acm uhci_hcd ehci_hcd i2c_dev i2c_i801 i2c_core msr cpuid ib_uverbs mlx4_ib ib_sa ib_mad mlx4_en ib_core mlx4_core ipv6^M
[   86.959307] CPU: 11 PID: 11378 Comm: BweTCTree_DoWor Not tainted 3.13.0-smp-DEV #413^M
[   86.973964] task: ffff880078fc6690 ti: ffff882016886000 task.ti: ffff882016886000^M
[   86.981467] RIP: 0010:[<ffffffff818a4b80>]  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   86.989153] RSP: 0018:ffff8820168879b0  EFLAGS: 00010206^M
[   86.994482] RAX: 0000000000010000 RBX: ffff881015c59480 RCX: 0000000000000000^M
[   87.001635] RDX: ffffffff818a4b50 RSI: ffffffffa0059790 RDI: ffff881015c59480^M
[   87.008772] RBP: ffff882016887aa8 R08: ffff88107f4004c0 R09: ffff881015c59480^M
[   87.015922] R10: ffff880000077680 R11: 00000000ffffff97 R12: ffff881016ae9160^M
[   87.023094] R13: ffff881016ae9000 R14: ffff881016ae9000 R15: ffff88102e405800^M
[   87.030277] FS:  00007f29f3858700(0000) GS:ffff88107fb60000(0000) knlGS:0000000000000000^M
[   87.038386] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033^M
[   87.044146] CR2: ffffffff818a4b80 CR3: 0000002030af1000 CR4: 00000000001407e0^M
[   87.051301] Stack:^M
[   87.053314]  ffffffff814dd0e1 0000000000000028 00000000ffffffff ffffffff818a4b50^M
[   87.060774]  ffff880000077680 ffff88102e405824 0000000800000000 ffffffff818a1940^M
[   87.068232]  0000000000000000 ffffffff8169ca80 0001000000010000 ffff882016887a68^M
[   87.075750] Call Trace:^M
[   87.078202]  [<ffffffff814dd0e1>] ? tc_ctl_tfilter+0x4a1/0x760^M
[   87.084071]  [<ffffffff814e6fb6>] ? __netlink_sendskb+0x56/0x130^M
[   87.090108]  [<ffffffff814caa04>] rtnetlink_rcv_msg+0xa4/0x240^M
[   87.095945]  [<ffffffff814ca960>] ? __rtnl_unlock+0x20/0x20^M
[   87.101538]  [<ffffffff814e9e91>] netlink_rcv_skb+0xb1/0xc0^M
[   87.107118]  [<ffffffff814c7635>] rtnetlink_rcv+0x25/0x40^M
[   87.112528]  [<ffffffff814e973d>] netlink_unicast+0x14d/0x1f0^M
[   87.118314]  [<ffffffff814e9ae3>] netlink_sendmsg+0x303/0x410^M
[   87.124092]  [<ffffffff814a048c>] sock_sendmsg+0x9c/0xd0^M
[   87.129451]  [<ffffffff814a0da0>] ? move_addr_to_kernel+0x40/0xa0^M
[   87.135566]  [<ffffffff814add21>] ? verify_iovec+0x51/0xd0^M
[   87.141064]  [<ffffffff814a0c21>] ___sys_sendmsg+0x3c1/0x3d0^M
[   87.146747]  [<ffffffff815690bc>] ? __do_page_fault+0x26c/0x500^M
[   87.152689]  [<ffffffff811821ab>] ? do_brk+0x1bb/0x320^M
[   87.157830]  [<ffffffff814a1809>] __sys_sendmsg+0x49/0x90^M
[   87.163250]  [<ffffffff814a1862>] SyS_sendmsg+0x12/0x20^M
[   87.168484]  [<ffffffff8156d992>] system_call_fastpath+0x16/0x1b^M
[   87.174518] Code: ff ff ff 80 97 05 a0 ff ff ff ff 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 80 4b 8a 81 ff ff ff ff 80 4b 8a 81 ff ff ff ff <70> 4b 8a 81 ff ff ff ff 70 4b 8a 81 ff ff ff ff c0 9d ba 81 ff ^M
[   87.194642] RIP  [<ffffffff818a4b80>] 0xffffffff818a4b80^M
[   87.199983]  RSP <ffff8820168879b0>^M
[   87.203472] CR2: ffffffff818a4b80^M
[   87.206785] ---[ end trace 8ef37a4cc4cb5a8c ]---^M
[   87.211408] Kernel panic - not syncing: Fatal exception^M
[   87.216763] Rebooting in 10 seconds..^M

We actually try to execute code at ffffffff818a4b80 == &act_mirred_ops

ffffffff818a4b40 d cls_mod_lock
ffffffff818a4b50 d tcf_proto_base
ffffffff818a4b60 d act_mod_lock
ffffffff818a4b70 d act_base
ffffffff818a4b80 d act_mirred_ops
ffffffff818a4bf0 d mirred_device_notifier
ffffffff818a4c10 d mirred_list
ffffffff818a4c20 d _rs.42828
ffffffff818a4c40 d ematch_mod_lock
ffffffff818a4c50 d ematch_ops

So only a bisection actually gave me the faulty commit.

Stacktrace was absolutely useless.

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Eric Dumazet]

On Fri, 2013-12-20 at 11:57 -0800, Cong Wang wrote:

> Sure, please fix act_api.c as well?

Yep.

This will be a separate patch, as the faulty commit is different.
( 1f747c26c48bb290c79c34e155860c7e2ec3926a net_sched: convert
tc_action_ops to use struct list_head)

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Jamal Hadi Salim]

On 12/20/13 15:14, Eric Dumazet wrote:

> This will be a separate patch, as the faulty commit is different.
> ( 1f747c26c48bb290c79c34e155860c7e2ec3926a net_sched: convert
> tc_action_ops to use struct list_head)
>

Ok, I hope Dave pulls those changes in - plan to pay closer attention
over the weekend.

cheers,
jamal

>
>

[PATCH net-next] net_sched: fix regression in tc_action_ops

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

list_for_each_entry(a, &act_base, head) doesn't
exit with a = NULL if we reached the end of the list.

tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
need fixes.

Remove tc_lookup_action_id() as its unused and not worth 'fixing'

Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")
Cc: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c |   53 ++++++++++--------------------------------
 1 file changed, 13 insertions(+), 40 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8114fef308d9..dce2b6ecdbd8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -291,12 +291,12 @@ int tcf_unregister_action(struct tc_action_ops *act)
 	int err = -ENOENT;
 
 	write_lock(&act_mod_lock);
-	list_for_each_entry(a, &act_base, head)
-		if (a == act)
+	list_for_each_entry(a, &act_base, head) {
+		if (a == act) {
+			list_del(&act->head);
+			err = 0;
 			break;
-	if (a) {
-		list_del(&act->head);
-		err = 0;
+		}
 	}
 	write_unlock(&act_mod_lock);
 	return err;
@@ -306,68 +306,41 @@ EXPORT_SYMBOL(tcf_unregister_action);
 /* lookup by name */
 static struct tc_action_ops *tc_lookup_action_n(char *kind)
 {
-	struct tc_action_ops *a = NULL;
+	struct tc_action_ops *a, *res = NULL;
 
 	if (kind) {
 		read_lock(&act_mod_lock);
 		list_for_each_entry(a, &act_base, head) {
 			if (strcmp(kind, a->kind) == 0) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
+				if (try_module_get(a->owner))
+					res = a;
 				break;
 			}
 		}
 		read_unlock(&act_mod_lock);
 	}
-	return a;
+	return res;
 }
 
 /* lookup by nlattr */
 static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
 {
-	struct tc_action_ops *a = NULL;
+	struct tc_action_ops *a, *res = NULL;
 
 	if (kind) {
 		read_lock(&act_mod_lock);
 		list_for_each_entry(a, &act_base, head) {
 			if (nla_strcmp(kind, a->kind) == 0) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
+				if (try_module_get(a->owner))
+					res = a;
 				break;
 			}
 		}
 		read_unlock(&act_mod_lock);
 	}
-	return a;
+	return res;
 }
 
-#if 0
-/* lookup by id */
-static struct tc_action_ops *tc_lookup_action_id(u32 type)
-{
-	struct tc_action_ops *a = NULL;
-
-	if (type) {
-		read_lock(&act_mod_lock);
-		for (a = act_base; a; a = a->next) {
-			if (a->type == type) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
-				break;
-			}
-		}
-		read_unlock(&act_mod_lock);
-	}
-	return a;
-}
-#endif
-
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res)
 {

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Fri, 20 Dec 2013 15:22:12 -0500

> On 12/20/13 15:14, Eric Dumazet wrote:
> 
>> This will be a separate patch, as the faulty commit is different.
>> ( 1f747c26c48bb290c79c34e155860c7e2ec3926a net_sched: convert
>> tc_action_ops to use struct list_head)
>>
> 
> Ok, I hope Dave pulls those changes in - plan to pay closer attention
> over the weekend.

I will.

Re: [PATCH net-next] net_sched: fix regression in tc_action_ops

[Cong Wang]

On Fri, Dec 20, 2013 at 12:32 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> list_for_each_entry(a, &act_base, head) doesn't
> exit with a = NULL if we reached the end of the list.
>
> tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
> need fixes.
>
> Remove tc_lookup_action_id() as its unused and not worth 'fixing'

I even don't want to touch the #if 0 code block. :)

>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")
> Cc: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[Cong Wang]

On Fri, Dec 20, 2013 at 12:11 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-12-20 at 11:57 -0800, Cong Wang wrote:
>
>> Then why mirred_cleanup_module() appears in the top of your backtrace?
>
> Because you have not understood the trace ?
>
> Thats exactly why its not useful at all.
>
>
> [   76.656061] RIP: 0010:[<ffffffffa008d882>]  [<ffffffffa008d882>] mirred_cleanup_module+0x13a/0x2a [act_mirred]^M
>
>
> This means RIP is _after_ mirred_cleanup_module() body, as this function size if 0x2a bytes
>
> We were trying to execute a data portion which happens to be in
> the neighboring of innocent mirred_cleanup_module()
>
> When not using module, trace was different and made no sense anyway :
>
...
>
> We actually try to execute code at ffffffff818a4b80 == &act_mirred_ops
>
> ffffffff818a4b40 d cls_mod_lock
> ffffffff818a4b50 d tcf_proto_base
> ffffffff818a4b60 d act_mod_lock
> ffffffff818a4b70 d act_base
> ffffffff818a4b80 d act_mirred_ops
> ffffffff818a4bf0 d mirred_device_notifier
> ffffffff818a4c10 d mirred_list
> ffffffff818a4c20 d _rs.42828
> ffffffff818a4c40 d ematch_mod_lock
> ffffffff818a4c50 d ematch_ops
>
> So only a bisection actually gave me the faulty commit.
>
> Stacktrace was absolutely useless.
>
>

Thanks for explaining this. Then:

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [Patch net-next] net_sched: fix a regression in tc actions

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 20 Dec 2013 00:08:51 -0800

> This patch fixes:
> 1) pass mask rather than size to tcf_hashinfo_init()
> 2) the cleanup should be in reversed order in mirred_cleanup_module()
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: 369ba56787d7469c0afd ("net_sched: init struct tcf_hashinfo at register time")
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

Re: [PATCH net-next] net_sched: fix a regression in tcf_proto_lookup_ops()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 20 Dec 2013 10:04:18 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> list_for_each_entry(t, &tcf_proto_base, head) doesn't
> exit with t = NULL if we reached the end of the list.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 3627287463b4 ("net_sched: convert tcf_proto_ops to use struct 
> list_head")
> Cc: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

Re: [PATCH net-next] net_sched: fix regression in tc_action_ops

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 20 Dec 2013 12:32:32 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> list_for_each_entry(a, &act_base, head) doesn't
> exit with a = NULL if we reached the end of the list.
> 
> tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
> need fixes.
> 
> Remove tc_lookup_action_id() as its unused and not worth 'fixing'
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")

Applied.