| Message ID | 71d1b5b90ab0e678be30726d373a9d325572a125.1539863681.git.dcaratti@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] net/sched: act_gact: properly init 'goto chain' | expand |
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...
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!
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.
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,
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
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(-)