Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Wed, 2018-10-17 at 22:35 -0700, Cong Wang wrote: > > (well, after some more thinking I looked again at that patch and yes, it > > lacked the most important thing:) > > Hmm, as I said, I am not sure if the logic is correct, if we have two > different > goto actions, we must have two pointers. > > I will re-think about it tomorrow. (I am at a conference so don't have much > time on reviewing this.) > > Thanks. sure, ok. In the meanwhile, I will post a V2 that: - adds the missing test that avoids having 'goto action' in the primary and in the fallback control action at the same time - fixes a very silly bug that made it fail the TDC 'gact' selftest regards, -- davide
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Tue, Oct 16, 2018 at 10:38 AM Davide Caratti wrote: > > On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote: > > On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti wrote: > > > > > > On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: > > > > Why not just validate the fallback action in each action init()? > > > > For example, checking tcfg_paction in tcf_gact_init(). > > > > > > > > I don't see the need of making it generic. > ... > > > A (legal?) trick is to let tcf_action store the fallback action when it > > > contains a 'goto chain' command, I just posted a proposal for gact. If you > > > think it's ok, I will test and post the same for act_police. > > > > Do we really need to support TC_ACT_GOTO_CHAIN for > > gact->tcfg_paction etc.? I mean, is it useful in practice or is it just for > > completeness? > > > > IF we don't need to support it, we can just make it invalid without needing > > to initialize it in ->init() at all. > > > > If we do, however, we really need to move it into each ->init(), because > > we have to lock each action if we are modifying an existing one. With > > your patch, tcf_action_goto_chain_init() is still called without the > > per-action > > lock. > > > > What's more, if we support two different actions in gact, that is, > > tcfg_paction > > and tcf_action, how could you still only have one a->goto_chain pointer? > > There should be two pointers for each of them. :) > > whatever fixes the NULL dereference is OK for me. > I thought that the proposal made with > > https://www.mail-archive.com/netdev@vger.kernel.org/msg251933.html > > (i.e., letting init() copy tcfg_paction to tcf_action in case it contained > 'goto chain x') was smart enough to preserve the current behavior, and > also let 'goto chain' work in case it was configured *only* for the > fallback action. > When the action is modified, the change to tcfg_paction is done with the > same spinlock as tcf_action, so I didn't notice anything worse than the > current locking layout. > > (well, after some more thinking I looked again at that patch and yes, it > lacked the most important thing:) Hmm, as I said, I am not sure if the logic is correct, if we have two different goto actions, we must have two pointers. I will re-think about it tomorrow. (I am at a conference so don't have much time on reviewing this.) Thanks.
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote: > On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti wrote: > > > > On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: > > > Why not just validate the fallback action in each action init()? > > > For example, checking tcfg_paction in tcf_gact_init(). > > > > > > I don't see the need of making it generic. ... > > A (legal?) trick is to let tcf_action store the fallback action when it > > contains a 'goto chain' command, I just posted a proposal for gact. If you > > think it's ok, I will test and post the same for act_police. > > Do we really need to support TC_ACT_GOTO_CHAIN for > gact->tcfg_paction etc.? I mean, is it useful in practice or is it just for > completeness? > > IF we don't need to support it, we can just make it invalid without needing > to initialize it in ->init() at all. > > If we do, however, we really need to move it into each ->init(), because > we have to lock each action if we are modifying an existing one. With > your patch, tcf_action_goto_chain_init() is still called without the > per-action > lock. > > What's more, if we support two different actions in gact, that is, > tcfg_paction > and tcf_action, how could you still only have one a->goto_chain pointer? > There should be two pointers for each of them. :) whatever fixes the NULL dereference is OK for me. I thought that the proposal made with https://www.mail-archive.com/netdev@vger.kernel.org/msg251933.html (i.e., letting init() copy tcfg_paction to tcf_action in case it contained 'goto chain x') was smart enough to preserve the current behavior, and also let 'goto chain' work in case it was configured *only* for the fallback action. When the action is modified, the change to tcfg_paction is done with the same spinlock as tcf_action, so I didn't notice anything worse than the current locking layout. (well, after some more thinking I looked again at that patch and yes, it lacked the most important thing:) --- a/net/sched/act_gact.c +++ b/net/sched/act_gact.c @@ -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 That said, 'goto chain' never worked for police and gact since the first introduction of 'goto chain', so we are not breaking any userspace program. And I don't necessarily need 'goto chain' in police and gact fallback actions; nobody complained in 1 year, so we can just add these two lines in tcf_gact_init() and something similar in tcf_police_init(): if (p_parm->ptype >= MAX_RAND) return -EINVAL; + if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN)) + return -EINVAL; (and maybe also help users with a proper extack). Just let me know which approach you prefer, I will test and send patches. thanks! -- davide
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti wrote: > > On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: > > Why not just validate the fallback action in each action init()? > > For example, checking tcfg_paction in tcf_gact_init(). > > > > I don't see the need of making it generic. > > hello Cong, once again thanks for looking at this. > > what you say is doable, and I evaluated doing it before proposing this > patch. > > But I felt unconfortable, because I needed to pass struct tcf_proto *tp in > tcf_gact_init() to initialize a->goto_chain with the chain_idx encoded in > the fallback action. So, I would have changed all the init() functions in > all TC actions, just to fix two of them. > > A (legal?) trick is to let tcf_action store the fallback action when it > contains a 'goto chain' command, I just posted a proposal for gact. If you > think it's ok, I will test and post the same for act_police. Do we really need to support TC_ACT_GOTO_CHAIN for gact->tcfg_paction etc.? I mean, is it useful in practice or is it just for completeness? IF we don't need to support it, we can just make it invalid without needing to initialize it in ->init() at all. If we do, however, we really need to move it into each ->init(), because we have to lock each action if we are modifying an existing one. With your patch, tcf_action_goto_chain_init() is still called without the per-action lock. What's more, if we support two different actions in gact, that is, tcfg_paction and tcf_action, how could you still only have one a->goto_chain pointer? There should be two pointers for each of them. :) Thanks.
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Sun, 2018-10-14 at 09:46 -0400, Jamal Hadi Salim wrote: > On 2018-10-13 11:23 a.m., Davide Caratti wrote: > > > > A (legal?) trick is to let tcf_action store the fallback action when it > > contains a 'goto chain' command, I just posted a proposal for gact. If you > > think it's ok, I will test and post the same for act_police. > > > > Need some more thought. So the issue here is the goto chain failed > the configured chain doesnt exist? 'goto chain' works only if it's stored in tcfa_action: if not, it does NULL dereference. That's ok for most actions, but not for gact and police - as they allow two control actions simultaneously, and the one that is stored in the action-specific data does not initialize any chain (because the initialization of 'goto_chain' data is done at [1]) (while at it, I also checked act_bpf, and it seems ok because 'goto chain' does not seem to be a valid control action for eBPF programs.) regards, -- davide [1] https://elixir.bootlin.com/linux/v4.19-rc7/source/net/sched/act_api.c#L888
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On 2018-10-13 11:23 a.m., Davide Caratti wrote: On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: Why not just validate the fallback action in each action init()? For example, checking tcfg_paction in tcf_gact_init(). I don't see the need of making it generic. hello Cong, once again thanks for looking at this. what you say is doable, and I evaluated doing it before proposing this patch. But I felt unconfortable, because I needed to pass struct tcf_proto *tp in tcf_gact_init() to initialize a->goto_chain with the chain_idx encoded in the fallback action. So, I would have changed all the init() functions in all TC actions, just to fix two of them. A (legal?) trick is to let tcf_action store the fallback action when it contains a 'goto chain' command, I just posted a proposal for gact. If you think it's ok, I will test and post the same for act_police. Need some more thought. So the issue here is the goto chain failed the configured chain doesnt exist? cheers, jamal
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: > Why not just validate the fallback action in each action init()? > For example, checking tcfg_paction in tcf_gact_init(). > > I don't see the need of making it generic. hello Cong, once again thanks for looking at this. what you say is doable, and I evaluated doing it before proposing this patch. But I felt unconfortable, because I needed to pass struct tcf_proto *tp in tcf_gact_init() to initialize a->goto_chain with the chain_idx encoded in the fallback action. So, I would have changed all the init() functions in all TC actions, just to fix two of them. A (legal?) trick is to let tcf_action store the fallback action when it contains a 'goto chain' command, I just posted a proposal for gact. If you think it's ok, I will test and post the same for act_police. regards, -- davide
Re: [PATCH net] net/sched: properly init chain in case of multiple control actions
On Fri, Oct 12, 2018 at 1:39 PM Davide Caratti wrote: > 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. Why not just validate the fallback action in each action init()? For example, checking tcfg_paction in tcf_gact_init(). I don't see the need of making it generic.
[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
PGD 0 P4D 0
Oops: [#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:9af96f843bf8 EFLAGS: 00010246
RAX: 202a RBX: 9af9679cf200 RCX: 005a
RDX: RSI: 0001 RDI: 9af585e006c0
RBP: 9af96f843ca0 R08: 1600 R09:
R10: R11: R12: 9af968db4400
R13: 9af968db4408 R14: 0001 R15: 9af585e006c0
FS: () GS:9af96f84() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: CR3: 00025980a001 CR4: 001606e0
Call Trace:
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
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:afa1832cbe90 EFLAGS: 0246 ORIG_RAX: ff13
RAX: 9af96f862600 RBX: 003ede349ac5 RCX: 001f
RDX: 003ede349ac5 RSI: 313b14ef RDI:
RBP: cfa17fa40a00 R08: 9af96f85cdc0 R09: afc8
R10: afa1832cbe70 R11: afc8 R12: 0004
R13: 82578bd8 R14: 003ec085dc50 R15:
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:
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
---
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
#include
-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)
{
-
