[PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[Davide Caratti]

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

when CONFIG_GACT_PROB is enabled, gact allows users to specify a fallback
control action, that is stored in the action private data. 'goto chain x'
never worked for that case, since goto_chain was never initialized. There
is only one goto_chain handle per action: ensure that gact never contains
more than one 'goto chain' in a rule. If the fallback control action is a
'goto chain', copy it to tcf_action to ensure that goto_chain is properly
initialized.

v2: - fix breakage of TDC tests when 'p_parm' is not specified
    - reject 'goto action' if it is specified twice in the same gact rule

Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/net/tc_act/tc_gact.h | 11 +++++++++--
 net/sched/act_gact.c         | 19 ++++++++++++++-----
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
index ef8dd0db70ce..0cbeb77349bc 100644
--- a/include/net/tc_act/tc_gact.h
+++ b/include/net/tc_act/tc_gact.h
@@ -10,12 +10,19 @@ struct tcf_gact {
 #ifdef CONFIG_GACT_PROB
 	u16			tcfg_ptype;
 	u16			tcfg_pval;
+	int			tcfg_caction;
 	int			tcfg_paction;
 	atomic_t		packets;
 #endif
 };
 #define to_gact(a) ((struct tcf_gact *)a)
 
+#ifdef CONFIG_GACT_PROB
+#define GACT_PRIMARY_ACTION(g) ((g)->tcfg_caction)
+#else
+#define GACT_PRIMARY_ACTION(g) ((g)->tcf_action)
+#endif
+
 static inline bool __is_tcf_gact_act(const struct tc_action *a, int act,
 				     bool is_ext)
 {
@@ -26,8 +33,8 @@ static inline bool __is_tcf_gact_act(const struct tc_action *a, int act,
 		return false;
 
 	gact = to_gact(a);
-	if ((!is_ext && gact->tcf_action == act) ||
-	    (is_ext && TC_ACT_EXT_CMP(gact->tcf_action, act)))
+	if ((!is_ext && GACT_PRIMARY_ACTION(gact) == act) ||
+	    (is_ext && TC_ACT_EXT_CMP(GACT_PRIMARY_ACTION(gact), act)))
 		return true;
 
 #endif
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index cd1d9bd32ef9..49b32650efb9 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -31,7 +31,7 @@ static int gact_net_rand(struct tcf_gact *gact)
 {
 	smp_rmb(); /* coupled with smp_wmb() in tcf_gact_init() */
 	if (prandom_u32() % gact->tcfg_pval)
-		return gact->tcf_action;
+		return gact->tcfg_caction;
 	return gact->tcfg_paction;
 }
 
@@ -41,7 +41,7 @@ static int gact_determ(struct tcf_gact *gact)
 
 	smp_rmb(); /* coupled with smp_wmb() in tcf_gact_init() */
 	if (pack % gact->tcfg_pval)
-		return gact->tcf_action;
+		return gact->tcfg_caction;
 	return gact->tcfg_paction;
 }
 
@@ -88,6 +88,9 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 		p_parm = nla_data(tb[TCA_GACT_PROB]);
 		if (p_parm->ptype >= MAX_RAND)
 			return -EINVAL;
+		if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN) &&
+		    TC_ACT_EXT_CMP(parm->action, TC_ACT_GOTO_CHAIN))
+			return -EINVAL;
 	}
 #endif
 
@@ -116,8 +119,14 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	spin_lock_bh(&gact->tcf_lock);
 	gact->tcf_action = parm->action;
 #ifdef CONFIG_GACT_PROB
+	gact->tcfg_caction = parm->action;
 	if (p_parm) {
 		gact->tcfg_paction = p_parm->paction;
+
+		/* ensure that 'goto_chain' is initialized correctly */
+		if (TC_ACT_EXT_CMP(gact->tcfg_paction, TC_ACT_GOTO_CHAIN))
+			gact->tcf_action = p_parm->paction;
+
 		gact->tcfg_pval    = max_t(u16, 1, p_parm->pval);
 		/* Make sure tcfg_pval is written before tcfg_ptype
 		 * coupled with smp_rmb() in gact_net_rand() & gact_determ()
@@ -137,7 +146,7 @@ static int tcf_gact_act(struct sk_buff *skb, const struct tc_action *a,
 			struct tcf_result *res)
 {
 	struct tcf_gact *gact = to_gact(a);
-	int action = READ_ONCE(gact->tcf_action);
+	int action = READ_ONCE(GACT_PRIMARY_ACTION(gact));
 
 #ifdef CONFIG_GACT_PROB
 	{
@@ -160,7 +169,7 @@ static void tcf_gact_stats_update(struct tc_action *a, u64 bytes, u32 packets,
 				  u64 lastuse)
 {
 	struct tcf_gact *gact = to_gact(a);
-	int action = READ_ONCE(gact->tcf_action);
+	int action = READ_ONCE(GACT_PRIMARY_ACTION(gact));
 	struct tcf_t *tm = &gact->tcf_tm;
 
 	_bstats_cpu_update(this_cpu_ptr(gact->common.cpu_bstats), bytes,
@@ -184,7 +193,7 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
 	struct tcf_t t;
 
 	spin_lock_bh(&gact->tcf_lock);
-	opt.action = gact->tcf_action;
+	opt.action = GACT_PRIMARY_ACTION(gact);
 	if (nla_put(skb, TCA_GACT_PARMS, sizeof(opt), &opt))
 		goto nla_put_failure;
 #ifdef CONFIG_GACT_PROB
-- 
2.17.1

Re: [PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[Jamal Hadi Salim]

On 2018-10-18 8:05 a.m., Davide Caratti wrote:
> 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
> 
> when CONFIG_GACT_PROB is enabled, gact allows users to specify a fallback
> control action, that is stored in the action private data. 'goto chain x'
> never worked for that case, since goto_chain was never initialized. There
> is only one goto_chain handle per action: ensure that gact never contains
> more than one 'goto chain' in a rule. If the fallback control action is a
> 'goto chain', copy it to tcf_action to ensure that goto_chain is properly
> initialized.
> 
> v2: - fix breakage of TDC tests when 'p_parm' is not specified
>      - reject 'goto action' if it is specified twice in the same gact rule
> 
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>   include/net/tc_act/tc_gact.h | 11 +++++++++--
>   net/sched/act_gact.c         | 19 ++++++++++++++-----
>   2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
> index ef8dd0db70ce..0cbeb77349bc 100644
> --- a/include/net/tc_act/tc_gact.h
> +++ b/include/net/tc_act/tc_gact.h
> @@ -10,12 +10,19 @@ struct tcf_gact {
>   #ifdef CONFIG_GACT_PROB
>   	u16			tcfg_ptype;
>   	u16			tcfg_pval;
> +	int			tcfg_caction;
>   	int			tcfg_paction;
>   	atomic_t		packets;
>   #endif
>   };
>   #define to_gact(a) ((struct tcf_gact *)a)
>   
> +#ifdef CONFIG_GACT_PROB
> +#define GACT_PRIMARY_ACTION(g) ((g)->tcfg_caction)
> +#else
> +#define GACT_PRIMARY_ACTION(g) ((g)->tcf_action)
> +#endif
> +
>   static inline bool __is_tcf_gact_act(const struct tc_action *a, int act,
>   				     bool is_ext)
>   {
> @@ -26,8 +33,8 @@ static inline bool __is_tcf_gact_act(const struct tc_action *a, int act,
>   		return false;
>   
>   	gact = to_gact(a);
> -	if ((!is_ext && gact->tcf_action == act) ||
> -	    (is_ext && TC_ACT_EXT_CMP(gact->tcf_action, act)))
> +	if ((!is_ext && GACT_PRIMARY_ACTION(gact) == act) ||
> +	    (is_ext && TC_ACT_EXT_CMP(GACT_PRIMARY_ACTION(gact), act)))
>   		return true;
>   
>   #endif
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index cd1d9bd32ef9..49b32650efb9 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -31,7 +31,7 @@ static int gact_net_rand(struct tcf_gact *gact)
>   {
>   	smp_rmb(); /* coupled with smp_wmb() in tcf_gact_init() */
>   	if (prandom_u32() % gact->tcfg_pval)
> -		return gact->tcf_action;
> +		return gact->tcfg_caction;
>   	return gact->tcfg_paction;
>   }
>   
> @@ -41,7 +41,7 @@ static int gact_determ(struct tcf_gact *gact)
>   
>   	smp_rmb(); /* coupled with smp_wmb() in tcf_gact_init() */
>   	if (pack % gact->tcfg_pval)
> -		return gact->tcf_action;
> +		return gact->tcfg_caction;
>   	return gact->tcfg_paction;
>   }
>   
> @@ -88,6 +88,9 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
>   		p_parm = nla_data(tb[TCA_GACT_PROB]);
>   		if (p_parm->ptype >= MAX_RAND)
>   			return -EINVAL;
> +		if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN) &&
> +		    TC_ACT_EXT_CMP(parm->action, TC_ACT_GOTO_CHAIN))
> +			return -EINVAL;

Rejection is a good solution[1].
Would be helpful to set an ext_ack to something like
"only one goto chain is supported currently"
I didnt follow why you needed to introduce tcfg_caction...

cheers,
jamal

[1]actions should allow multitude return opcodes, not
just two. The proper solution seems to be to just let
the caller (cls_api - who is aware of chains) to
deal with the chain selection. Sticking them in actions
speeds up lookup (and deal with refcnts) but i wonder
given this breakage in abstraction whether they belong
there...

Re: [PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[Davide Caratti]

hello Jamal,

On Thu, 2018-10-18 at 08:52 -0400, Jamal Hadi Salim wrote:
> Rejection is a good solution[1].
> Would be helpful to set an ext_ack to something like
> "only one goto chain is supported currently"

ok, that's material for a v3, if you / Cong / others are ok with the other
hunks.

On Thu, 2018-10-18 at 08:52 -0400, Jamal Hadi Salim wrote:
> I didnt follow why you needed to introduce tcfg_caction...

it's a trick (maybe an abuse, up to you to decide): when the user does

 # tc f a dev v0 egress matchall action pass random determ goto chain 4 5

tcfg_caction contains 'pass', tcfg_paction and tcf_action both contain
'goto chain 4'. On the opposite, when the user does

 # tc f a dev v0 egress matchall action goto chain 4 random determ drop 5

tcfg_caction and tcf_action both contain 'goto chain 4', and tcf_paction
contains 'drop'. In this way, the 'goto chain' is always stored in
tcf_action, so that TC generic code can always initialize a->goto_chain.
(we need tcf_caction, becaue tcf_gact_dump() and helpers in tc_gact.h need
to read the control action on the left of "random")

The alternative is, we systematically forbid usage of 'goto chain' in
tcfg_paction, so that:

# tc f a dev v0 egress matchall action <whatever> random determ goto chain 4 5

is systematically rejected with -EINVAL. This comand never worked, so we
are not breaking anything in userspace.

thanks for reviewing!
-- 
davide

Re: [PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[Cong Wang]

On Thu, Oct 18, 2018 at 8:30 AM Davide Caratti <dcaratti@redhat.com> wrote:
> The alternative is, we systematically forbid usage of 'goto chain' in
> tcfg_paction, so that:
>
> # tc f a dev v0 egress matchall action <whatever> random determ goto chain 4 5
>
> is systematically rejected with -EINVAL. This comand never worked, so we
> are not breaking anything in userspace.

This is exactly why I asked you if we really need to support it. :)

If no one finds it useful, disallowing it is a good solution here, as
we don't need
to introduce any additional code to handle filter chains.

Re: [PATCH net v2] net/sched: act_gact: properly init 'goto chain'

[Davide Caratti]

hello Cong and Jamal,

On Fri, 2018-10-19 at 13:40 -0700, Cong Wang wrote:
> On Thu, Oct 18, 2018 at 8:30 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > The alternative is, we systematically forbid usage of 'goto chain' in
> > tcfg_paction, so that:
> > 
> > # tc f a dev v0 egress matchall action <whatever> random determ goto chain 4 5
> > 
> > is systematically rejected with -EINVAL. This comand never worked, so we
> > are not breaking anything in userspace.

> This is exactly why I asked you if we really need to support it. :)
> 
> If no one finds it useful, disallowing it is a good solution here, as
> we don't need
> to introduce any additional code to handle filter chains.

On Thu, 2018-10-18 at 08:52 -0400, Jamal Hadi Salim wrote:

> Rejection is a good solution[1].
> Would be helpful to set an ext_ack to something like
> "only one goto chain is supported currently"

OK to forbid 'goto chain' on fallback actions for gact and police: I just
sent out a small series for that, feedbacks are welcome.

@David: this patch is no more needed, it can be dropped from patchwork.

thanks!
regards,
-- 
davide