Date:   Fri, 12 Oct 2018 22:39:21 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: [PATCH net] net/sched: properly init chain in case of multiple control actions

the following script:

 # tc f a dev v0 egress chain 4 matchall action simple sdata "A triumph!"
 # tc f a dev v0 egress matchall action pass random determ goto chain 4 5

produces the following crash:

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
 PGD 0 P4D 0
 Oops: 0000 [#1] SMP PTI
 CPU: 9 PID: 0 Comm: swapper/9 Not tainted 4.19.0-rc6.chainfix + #472
 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
 RIP: 0010:tcf_action_exec+0xb8/0x100
 Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
 RSP: 0018:ffff9af96f843bf8 EFLAGS: 00010246
 RAX: 000000002000002a RBX: ffff9af9679cf200 RCX: 000000000000005a
 RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff9af585e006c0
 RBP: ffff9af96f843ca0 R08: 0000000016000000 R09: 0000000000000000
 R10: 0000000000000000 R11: 0000000000000000 R12: ffff9af968db4400
 R13: ffff9af968db4408 R14: 0000000000000001 R15: ffff9af585e006c0
 FS:  0000000000000000(0000) GS:ffff9af96f840000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 000000025980a001 CR4: 00000000001606e0
 Call Trace:
  <IRQ>
  tcf_classify+0x89/0x140
  __dev_queue_xmit+0x413/0x8a0
  ? ip6_finish_output2+0x336/0x520
  ip6_finish_output2+0x336/0x520
  ? ip6_output+0x68/0x110
  ip6_output+0x68/0x110
  ? ip6_fragment+0x9e0/0x9e0
  mld_sendpack+0x175/0x220
  ? mld_gq_timer_expire+0x40/0x40
  mld_dad_timer_expire+0x25/0x80
  call_timer_fn+0x2b/0x120
  run_timer_softirq+0x3e8/0x440
  ? tick_sched_timer+0x37/0x70
  ? __hrtimer_run_queues+0x118/0x290
  __do_softirq+0xe3/0x2bd
  irq_exit+0xe3/0xf0
  smp_apic_timer_interrupt+0x74/0x130
  apic_timer_interrupt+0xf/0x20
  </IRQ>
 RIP: 0010:cpuidle_enter_state+0xa5/0x320
 Code: 71 82 5f 7e e8 bc 25 ab ff 48 89 c3 0f 1f 44 00 00 31 ff e8 3d 36 ab ff 80 7c 24 07 00 0f 85 28 02 00 00 fb 66 0f 1f 44 00 00 <4c> 29 f3 48 ba cf f7 53 e3 a5 9b c4 20 48 89 d8 48 c1 fb 3f 48 f7
 RSP: 0018:ffffafa1832cbe90 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
 RAX: ffff9af96f862600 RBX: 0000003ede349ac5 RCX: 000000000000001f
 RDX: 0000003ede349ac5 RSI: 00000000313b14ef RDI: 0000000000000000
 RBP: ffffcfa17fa40a00 R08: ffff9af96f85cdc0 R09: 000000000000afc8
 R10: ffffafa1832cbe70 R11: 000000000000afc8 R12: 0000000000000004
 R13: ffffffff82578bd8 R14: 0000003ec085dc50 R15: 0000000000000000
  do_idle+0x200/0x280
  cpu_startup_entry+0x6f/0x80
  start_secondary+0x1a7/0x200
  secondary_startup_64+0xa4/0xb0
 Modules linked in: act_gact act_simple cls_matchall sch_ingress veth intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ipmi_ssif ghash_clmulni_intel pcbc aesni_intel ipmi_si iTCO_wdt crypto_simd iTCO_vendor_support cryptd mei_me ipmi_devintf glue_helper mei joydev ipmi_msghandler pcc_cpufreq sg lpc_ich pcspkr i2c_i801 ioatdma wmi ip_tables xfs libcrc32c mlx4_en sd_mod mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm isci drm libsas igb ahci libahci scsi_transport_sas mlx4_core be2net crc32c_intel dca libata i2c_algo_bit i2c_core megaraid_sas devlink dm_mirror dm_region_hash dm_log dm_mod
 CR2: 0000000000000000

Several TC actions allow users to specify a fallback control action, that
is usually stored in the action private data. 'goto chain x' never worked
for that case, because the action handler was never initialized. There is
only one 'goto_chain' handle per action: extend act_api to disallow 'goto
chain' specified more than once in a rule. If the fallback control action
is legally configured, use it to properly initialize the chain.

Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Signed-off-by: Davide Caratti <dcaratti@...hat.com>
---
 include/net/act_api.h  |  1 +
 net/sched/act_api.c    | 28 +++++++++++++++++++++++-----
 net/sched/act_gact.c   |  8 ++++++++
 net/sched/act_police.c | 13 +++++++++++++
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 970303448c90..efc2309a6545 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -99,6 +99,7 @@ struct tc_action_ops {
 	size_t  (*get_fill_size)(const struct tc_action *act);
 	struct net_device *(*get_dev)(const struct tc_action *a);
 	void	(*put_dev)(struct net_device *dev);
+	int	(*fallback_act)(const struct tc_action *a);
 };
 
 struct tc_action_net {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e12f8ef7baa4..3eaa61abf190 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -30,10 +30,9 @@
 #include <net/act_api.h>
 #include <net/netlink.h>
 
-static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
+static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp,
+				      u32 chain_index)
 {
-	u32 chain_index = a->tcfa_action & TC_ACT_EXT_VAL_MASK;
-
 	if (!tp)
 		return -EINVAL;
 	a->goto_chain = tcf_chain_get_by_act(tp->chain->block, chain_index);
@@ -798,7 +797,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	struct tc_cookie *cookie = NULL;
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
+	bool do_init_chain = false;
 	struct nlattr *kind;
+	u32 chain_id;
 	int err;
 
 	if (name == NULL) {
@@ -886,7 +887,23 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		module_put(a_o->owner);
 
 	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
-		err = tcf_action_goto_chain_init(a, tp);
+		do_init_chain = true;
+		chain_id = a->tcfa_action & TC_ACT_EXT_VAL_MASK;
+		if (a_o->fallback_act && TC_ACT_EXT_CMP(a_o->fallback_act(a),
+							TC_ACT_GOTO_CHAIN)) {
+			NL_SET_ERR_MSG(extack, "Too many 'goto chain'");
+			return ERR_PTR(-EINVAL);
+		}
+	} else if (a_o->fallback_act) {
+		chain_id = a_o->fallback_act(a);
+		if (TC_ACT_EXT_CMP(chain_id, TC_ACT_GOTO_CHAIN)) {
+			do_init_chain = true;
+			chain_id &= TC_ACT_EXT_VAL_MASK;
+		}
+	}
+
+	if (do_init_chain) {
+		err = tcf_action_goto_chain_init(a, tp, chain_id);
 		if (err) {
 			tcf_action_destroy_1(a, bind);
 			NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
@@ -894,7 +911,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
-	if (!tcf_action_valid(a->tcfa_action)) {
+	if (!tcf_action_valid(a->tcfa_action) ||
+	    (a_o->fallback_act && !tcf_action_valid(a_o->fallback_act(a)))) {
 		tcf_action_destroy_1(a, bind);
 		NL_SET_ERR_MSG(extack, "Invalid control action value");
 		return ERR_PTR(-EINVAL);
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index cd1d9bd32ef9..77554e87d658 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -47,6 +47,11 @@ static int gact_determ(struct tcf_gact *gact)
 
 typedef int (*g_rand)(struct tcf_gact *gact);
 static g_rand gact_rand[MAX_RAND] = { NULL, gact_net_rand, gact_determ };
+
+static int tcf_gact_fallback_action(const struct tc_action *act)
+{
+	return to_gact(act)->tcfg_paction;
+}
 #endif /* CONFIG_GACT_PROB */
 
 static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
@@ -254,6 +259,9 @@ static struct tc_action_ops act_gact_ops = {
 	.walk		=	tcf_gact_walker,
 	.lookup		=	tcf_gact_search,
 	.get_fill_size	=	tcf_gact_get_fill_size,
+#ifdef CONFIG_GACT_PROB
+	.fallback_act	=	tcf_gact_fallback_action,
+#endif
 	.size		=	sizeof(struct tcf_gact),
 };
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 5d8bfa878477..03ecb063c415 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -320,6 +320,18 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
+static int tcf_police_fallback_action(const struct tc_action *a)
+{
+	struct tcf_police *police = to_police(a);
+	int retval;
+
+	spin_lock_bh(&police->tcf_lock);
+	retval =  police->tcfp_result;
+	spin_unlock_bh(&police->tcf_lock);
+
+	return retval;
+}
+
 MODULE_AUTHOR("Alexey Kuznetsov");
 MODULE_DESCRIPTION("Policing actions");
 MODULE_LICENSE("GPL");
@@ -333,6 +345,7 @@ static struct tc_action_ops act_police_ops = {
 	.init		=	tcf_police_init,
 	.walk		=	tcf_police_walker,
 	.lookup		=	tcf_police_search,
+	.fallback_act	=	tcf_police_fallback_action,
 	.size		=	sizeof(struct tcf_police),
 };
 
-- 
2.17.1

Powered by blists - more mailing lists