[PATCH net] net/sched: properly init chain in case of multiple control actions

[PATCH net] net/sched: properly init chain in case of multiple control actions

[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

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@redhat.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

Re: [PATCH net] net/sched: properly init chain in case of multiple control actions

[Cong Wang]

On Fri, Oct 12, 2018 at 1:39 PM Davide Caratti <dcaratti@redhat.com> 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.

Re: [PATCH net] net/sched: properly init chain in case of multiple control actions

[Davide Caratti]

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

[Jamal Hadi Salim]

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

[Davide Caratti]

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

[Cong Wang]

On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcaratti@redhat.com> 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

[Davide Caratti]

On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote:
> On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcaratti@redhat.com> 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

[Cong Wang]

On Tue, Oct 16, 2018 at 10:38 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote:
> > On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcaratti@redhat.com> 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

[Davide Caratti]

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