| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b27083a6-ad14-0f44-f85c-1ee73b8ddb8a@mojatatu.com>
Date: Thu, 18 Oct 2018 08:52:45 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Davide Caratti <dcaratti@...hat.com>,
Jiri Pirko <jiri@...nulli.us>,
Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] net/sched: act_gact: properly init 'goto chain'
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@...hat.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...
Powered by blists - more mailing lists