[PATCH net v3 0/2] two fixes for 'act_gate' control plane

[PATCH net v3 0/2] two fixes for 'act_gate' control plane

[Davide Caratti]

- patch 1/2 attempts to fix the error path of tcf_gate_init() when users
  try to configure 'act_gate' rules with wrong parameters
- patch 2/2 is a follow-up of a recent fix for NULL dereference in
  the error path of tcf_gate_init()

further work will introduce a tdc test for 'act_gate'.

changes since v2:
  - fix undefined behavior in patch 1/2
  - improve comment in patch 2/2
changes since v1:
  coding style fixes in patch 1/2 and 2/2

Davide Caratti (2):
  net/sched: act_gate: fix NULL dereference in tcf_gate_init()
  net/sched: act_gate: fix configuration of the periodic timer

 net/sched/act_gate.c | 126 +++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 58 deletions(-)

-- 
2.26.2

[PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init()

[Davide Caratti]

it is possible to see a KASAN use-after-free, immediately followed by a
NULL dereference crash, with the following command:

 # tc action add action gate index 3 cycle-time 100000000ns \
 > cycle-time-ext 100000000ns clockid CLOCK_TAI

 BUG: KASAN: use-after-free in tcf_action_init_1+0x8eb/0x960
 Write of size 1 at addr ffff88810a5908bc by task tc/883

 CPU: 0 PID: 883 Comm: tc Not tainted 5.7.0+ #188
 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
 Call Trace:
  dump_stack+0x75/0xa0
  print_address_description.constprop.6+0x1a/0x220
  kasan_report.cold.9+0x37/0x7c
  tcf_action_init_1+0x8eb/0x960
  tcf_action_init+0x157/0x2a0
  tcf_action_add+0xd9/0x2f0
  tc_ctl_action+0x2a3/0x39d
  rtnetlink_rcv_msg+0x5f3/0x920
  netlink_rcv_skb+0x120/0x380
  netlink_unicast+0x439/0x630
  netlink_sendmsg+0x714/0xbf0
  sock_sendmsg+0xe2/0x110
  ____sys_sendmsg+0x5b4/0x890
  ___sys_sendmsg+0xe9/0x160
  __sys_sendmsg+0xd3/0x170
  do_syscall_64+0x9a/0x370
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

[...]

 KASAN: null-ptr-deref in range [0x0000000000000070-0x0000000000000077]
 CPU: 0 PID: 883 Comm: tc Tainted: G    B             5.7.0+ #188
 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
 RIP: 0010:tcf_action_fill_size+0xa3/0xf0
 [....]
 RSP: 0018:ffff88813a48f250 EFLAGS: 00010212
 RAX: dffffc0000000000 RBX: 0000000000000094 RCX: ffffffffa47c3eb6
 RDX: 000000000000000e RSI: 0000000000000008 RDI: 0000000000000070
 RBP: ffff88810a590800 R08: 0000000000000004 R09: ffffed1027491e03
 R10: 0000000000000003 R11: ffffed1027491e03 R12: 0000000000000000
 R13: 0000000000000000 R14: dffffc0000000000 R15: ffff88810a590800
 FS:  00007f62cae8ce40(0000) GS:ffff888147c00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f62c9d20a10 CR3: 000000013a52a000 CR4: 0000000000340ef0
 Call Trace:
  tcf_action_init+0x172/0x2a0
  tcf_action_add+0xd9/0x2f0
  tc_ctl_action+0x2a3/0x39d
  rtnetlink_rcv_msg+0x5f3/0x920
  netlink_rcv_skb+0x120/0x380
  netlink_unicast+0x439/0x630
  netlink_sendmsg+0x714/0xbf0
  sock_sendmsg+0xe2/0x110
  ____sys_sendmsg+0x5b4/0x890
  ___sys_sendmsg+0xe9/0x160
  __sys_sendmsg+0xd3/0x170
  do_syscall_64+0x9a/0x370
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

this is caused by the test on 'cycletime_ext', that is still unassigned
when the action is newly created. This makes the action .init() return 0
without calling tcf_idr_insert(), hence the UAF + crash.

rework the logic that prevents zero values of cycle-time, as follows:

1) 'tcfg_cycletime_ext' seems to be unused in the action software path,
   and it was already possible by other means to obtain non-zero
   cycletime and zero cycletime-ext. So, removing that test should not
   cause any damage.
2) while at it, we must prevent overwriting configuration data with wrong
   ones: use a temporary variable for 'tcfg_cycletime', and validate it
   preserving the original semantic (that allowed computing the cycle
   time as the sum of all intervals, when not specified by
   TCA_GATE_CYCLE_TIME).
3) remove the test on 'tcfg_cycletime', no more useful, and avoid
   returning -EFAULT, which did not seem an appropriate return value for
   a wrong netlink attribute.

v3: fix uninitialized 'cycletime' (thanks to Vladimir Oltean)
v2: remove useless 'return;' at the end of void gate_get_start_time()

Fixes: a51c328df310 ("net: qos: introduce a gate control flow action")
CC: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/act_gate.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 9c628591f452..94e560c2f81d 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -32,7 +32,7 @@ static ktime_t gate_get_time(struct tcf_gate *gact)
 	return KTIME_MAX;
 }
 
-static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
+static void gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
 {
 	struct tcf_gate_params *param = &gact->param;
 	ktime_t now, base, cycle;
@@ -43,18 +43,13 @@ static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
 
 	if (ktime_after(base, now)) {
 		*start = base;
-		return 0;
+		return;
 	}
 
 	cycle = param->tcfg_cycletime;
 
-	/* cycle time should not be zero */
-	if (!cycle)
-		return -EFAULT;
-
 	n = div64_u64(ktime_sub_ns(now, base), cycle);
 	*start = ktime_add_ns(base, (n + 1) * cycle);
-	return 0;
 }
 
 static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
@@ -287,12 +282,12 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	enum tk_offsets tk_offset = TK_OFFS_TAI;
 	struct nlattr *tb[TCA_GATE_MAX + 1];
 	struct tcf_chain *goto_ch = NULL;
+	u64 cycletime = 0, basetime = 0;
 	struct tcf_gate_params *p;
 	s32 clockid = CLOCK_TAI;
 	struct tcf_gate *gact;
 	struct tc_gate *parm;
 	int ret = 0, err;
-	u64 basetime = 0;
 	u32 gflags = 0;
 	s32 prio = -1;
 	ktime_t start;
@@ -375,11 +370,8 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	spin_lock_bh(&gact->tcf_lock);
 	p = &gact->param;
 
-	if (tb[TCA_GATE_CYCLE_TIME]) {
-		p->tcfg_cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
-		if (!p->tcfg_cycletime_ext)
-			goto chain_put;
-	}
+	if (tb[TCA_GATE_CYCLE_TIME])
+		cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
 
 	if (tb[TCA_GATE_ENTRY_LIST]) {
 		err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
@@ -387,14 +379,19 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 			goto chain_put;
 	}
 
-	if (!p->tcfg_cycletime) {
+	if (!cycletime) {
 		struct tcfg_gate_entry *entry;
 		ktime_t cycle = 0;
 
 		list_for_each_entry(entry, &p->entries, list)
 			cycle = ktime_add_ns(cycle, entry->interval);
-		p->tcfg_cycletime = cycle;
+		cycletime = cycle;
+		if (!cycletime) {
+			err = -EINVAL;
+			goto chain_put;
+		}
 	}
+	p->tcfg_cycletime = cycletime;
 
 	if (tb[TCA_GATE_CYCLE_TIME_EXT])
 		p->tcfg_cycletime_ext =
@@ -408,14 +405,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	gact->tk_offset = tk_offset;
 	hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
 	gact->hitimer.function = gate_timer_func;
-
-	err = gate_get_start_time(gact, &start);
-	if (err < 0) {
-		NL_SET_ERR_MSG(extack,
-			       "Internal error: failed get start time");
-		release_entry_list(&p->entries);
-		goto chain_put;
-	}
+	gate_get_start_time(gact, &start);
 
 	gact->current_close_time = start;
 	gact->current_gate_status = GATE_ACT_GATE_OPEN | GATE_ACT_PENDING;
-- 
2.26.2

Re: [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init()

[Vladimir Oltean]

On Tue, 16 Jun 2020 at 23:25, Davide Caratti <dcaratti@redhat.com> wrote:
>
> it is possible to see a KASAN use-after-free, immediately followed by a
> NULL dereference crash, with the following command:
>
>  # tc action add action gate index 3 cycle-time 100000000ns \
>  > cycle-time-ext 100000000ns clockid CLOCK_TAI
>
>  BUG: KASAN: use-after-free in tcf_action_init_1+0x8eb/0x960
>  Write of size 1 at addr ffff88810a5908bc by task tc/883
>
>  CPU: 0 PID: 883 Comm: tc Not tainted 5.7.0+ #188
>  Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
>  Call Trace:
>   dump_stack+0x75/0xa0
>   print_address_description.constprop.6+0x1a/0x220
>   kasan_report.cold.9+0x37/0x7c
>   tcf_action_init_1+0x8eb/0x960
>   tcf_action_init+0x157/0x2a0
>   tcf_action_add+0xd9/0x2f0
>   tc_ctl_action+0x2a3/0x39d
>   rtnetlink_rcv_msg+0x5f3/0x920
>   netlink_rcv_skb+0x120/0x380
>   netlink_unicast+0x439/0x630
>   netlink_sendmsg+0x714/0xbf0
>   sock_sendmsg+0xe2/0x110
>   ____sys_sendmsg+0x5b4/0x890
>   ___sys_sendmsg+0xe9/0x160
>   __sys_sendmsg+0xd3/0x170
>   do_syscall_64+0x9a/0x370
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [...]
>
>  KASAN: null-ptr-deref in range [0x0000000000000070-0x0000000000000077]
>  CPU: 0 PID: 883 Comm: tc Tainted: G    B             5.7.0+ #188
>  Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
>  RIP: 0010:tcf_action_fill_size+0xa3/0xf0
>  [....]
>  RSP: 0018:ffff88813a48f250 EFLAGS: 00010212
>  RAX: dffffc0000000000 RBX: 0000000000000094 RCX: ffffffffa47c3eb6
>  RDX: 000000000000000e RSI: 0000000000000008 RDI: 0000000000000070
>  RBP: ffff88810a590800 R08: 0000000000000004 R09: ffffed1027491e03
>  R10: 0000000000000003 R11: ffffed1027491e03 R12: 0000000000000000
>  R13: 0000000000000000 R14: dffffc0000000000 R15: ffff88810a590800
>  FS:  00007f62cae8ce40(0000) GS:ffff888147c00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f62c9d20a10 CR3: 000000013a52a000 CR4: 0000000000340ef0
>  Call Trace:
>   tcf_action_init+0x172/0x2a0
>   tcf_action_add+0xd9/0x2f0
>   tc_ctl_action+0x2a3/0x39d
>   rtnetlink_rcv_msg+0x5f3/0x920
>   netlink_rcv_skb+0x120/0x380
>   netlink_unicast+0x439/0x630
>   netlink_sendmsg+0x714/0xbf0
>   sock_sendmsg+0xe2/0x110
>   ____sys_sendmsg+0x5b4/0x890
>   ___sys_sendmsg+0xe9/0x160
>   __sys_sendmsg+0xd3/0x170
>   do_syscall_64+0x9a/0x370
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> this is caused by the test on 'cycletime_ext', that is still unassigned
> when the action is newly created. This makes the action .init() return 0
> without calling tcf_idr_insert(), hence the UAF + crash.
>
> rework the logic that prevents zero values of cycle-time, as follows:
>
> 1) 'tcfg_cycletime_ext' seems to be unused in the action software path,
>    and it was already possible by other means to obtain non-zero
>    cycletime and zero cycletime-ext. So, removing that test should not
>    cause any damage.
> 2) while at it, we must prevent overwriting configuration data with wrong
>    ones: use a temporary variable for 'tcfg_cycletime', and validate it
>    preserving the original semantic (that allowed computing the cycle
>    time as the sum of all intervals, when not specified by
>    TCA_GATE_CYCLE_TIME).
> 3) remove the test on 'tcfg_cycletime', no more useful, and avoid
>    returning -EFAULT, which did not seem an appropriate return value for
>    a wrong netlink attribute.
>
> v3: fix uninitialized 'cycletime' (thanks to Vladimir Oltean)
> v2: remove useless 'return;' at the end of void gate_get_start_time()
>
> Fixes: a51c328df310 ("net: qos: introduce a gate control flow action")
> CC: Ivan Vecera <ivecera@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>


>  net/sched/act_gate.c | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index 9c628591f452..94e560c2f81d 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> @@ -32,7 +32,7 @@ static ktime_t gate_get_time(struct tcf_gate *gact)
>         return KTIME_MAX;
>  }
>
> -static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
> +static void gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
>  {
>         struct tcf_gate_params *param = &gact->param;
>         ktime_t now, base, cycle;
> @@ -43,18 +43,13 @@ static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
>
>         if (ktime_after(base, now)) {
>                 *start = base;
> -               return 0;
> +               return;
>         }
>
>         cycle = param->tcfg_cycletime;
>
> -       /* cycle time should not be zero */
> -       if (!cycle)
> -               return -EFAULT;
> -
>         n = div64_u64(ktime_sub_ns(now, base), cycle);
>         *start = ktime_add_ns(base, (n + 1) * cycle);
> -       return 0;
>  }
>
>  static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
> @@ -287,12 +282,12 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         enum tk_offsets tk_offset = TK_OFFS_TAI;
>         struct nlattr *tb[TCA_GATE_MAX + 1];
>         struct tcf_chain *goto_ch = NULL;
> +       u64 cycletime = 0, basetime = 0;
>         struct tcf_gate_params *p;
>         s32 clockid = CLOCK_TAI;
>         struct tcf_gate *gact;
>         struct tc_gate *parm;
>         int ret = 0, err;
> -       u64 basetime = 0;
>         u32 gflags = 0;
>         s32 prio = -1;
>         ktime_t start;
> @@ -375,11 +370,8 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         spin_lock_bh(&gact->tcf_lock);
>         p = &gact->param;
>
> -       if (tb[TCA_GATE_CYCLE_TIME]) {
> -               p->tcfg_cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
> -               if (!p->tcfg_cycletime_ext)
> -                       goto chain_put;
> -       }
> +       if (tb[TCA_GATE_CYCLE_TIME])
> +               cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
>
>         if (tb[TCA_GATE_ENTRY_LIST]) {
>                 err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
> @@ -387,14 +379,19 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>                         goto chain_put;
>         }
>
> -       if (!p->tcfg_cycletime) {
> +       if (!cycletime) {
>                 struct tcfg_gate_entry *entry;
>                 ktime_t cycle = 0;
>
>                 list_for_each_entry(entry, &p->entries, list)
>                         cycle = ktime_add_ns(cycle, entry->interval);
> -               p->tcfg_cycletime = cycle;
> +               cycletime = cycle;
> +               if (!cycletime) {
> +                       err = -EINVAL;
> +                       goto chain_put;
> +               }
>         }
> +       p->tcfg_cycletime = cycletime;
>
>         if (tb[TCA_GATE_CYCLE_TIME_EXT])
>                 p->tcfg_cycletime_ext =
> @@ -408,14 +405,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         gact->tk_offset = tk_offset;
>         hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
>         gact->hitimer.function = gate_timer_func;
> -
> -       err = gate_get_start_time(gact, &start);
> -       if (err < 0) {
> -               NL_SET_ERR_MSG(extack,
> -                              "Internal error: failed get start time");
> -               release_entry_list(&p->entries);
> -               goto chain_put;
> -       }
> +       gate_get_start_time(gact, &start);
>
>         gact->current_close_time = start;
>         gact->current_gate_status = GATE_ACT_GATE_OPEN | GATE_ACT_PENDING;
> --
> 2.26.2
>

[PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer

[Davide Caratti]

assigning a dummy value of 'clock_id' to avoid cancellation of the cycle
timer before its initialization was a temporary solution, and we still
need to handle the case where act_gate timer parameters are changed by
commands like the following one:

 # tc action replace action gate <parameters>

the fix consists in the following items:

1) remove the workaround assignment of 'clock_id', and init the list of
   entries before the first error path after IDR atomic check/allocation
2) validate 'clock_id' earlier: there is no need to do IDR atomic
   check/allocation if we know that 'clock_id' is a bad value
3) use a dedicated function, 'gate_setup_timer()', to ensure that the
   timer is cancelled and re-initialized on action overwrite, and also
   ensure we initialize the timer in the error path of tcf_gate_init()

v3: improve comment in the error path of tcf_gate_init() (thanks to
    Vladimir Oltean)
v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)

CC: Ivan Vecera <ivecera@redhat.com>
Fixes: a01c245438c5 ("net/sched: fix a couple of splats in the error path of tfc_gate_init()")
Fixes: a51c328df310 ("net: qos: introduce a gate control flow action")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/act_gate.c | 90 +++++++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 94e560c2f81d..323ae7f6315d 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -272,6 +272,27 @@ static int parse_gate_list(struct nlattr *list_attr,
 	return err;
 }
 
+static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
+			     enum tk_offsets tko, s32 clockid,
+			     bool do_init)
+{
+	if (!do_init) {
+		if (basetime == gact->param.tcfg_basetime &&
+		    tko == gact->tk_offset &&
+		    clockid == gact->param.tcfg_clockid)
+			return;
+
+		spin_unlock_bh(&gact->tcf_lock);
+		hrtimer_cancel(&gact->hitimer);
+		spin_lock_bh(&gact->tcf_lock);
+	}
+	gact->param.tcfg_basetime = basetime;
+	gact->param.tcfg_clockid = clockid;
+	gact->tk_offset = tko;
+	hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
+	gact->hitimer.function = gate_timer_func;
+}
+
 static int tcf_gate_init(struct net *net, struct nlattr *nla,
 			 struct nlattr *est, struct tc_action **a,
 			 int ovr, int bind, bool rtnl_held,
@@ -303,6 +324,27 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	if (!tb[TCA_GATE_PARMS])
 		return -EINVAL;
 
+	if (tb[TCA_GATE_CLOCKID]) {
+		clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
+		switch (clockid) {
+		case CLOCK_REALTIME:
+			tk_offset = TK_OFFS_REAL;
+			break;
+		case CLOCK_MONOTONIC:
+			tk_offset = TK_OFFS_MAX;
+			break;
+		case CLOCK_BOOTTIME:
+			tk_offset = TK_OFFS_BOOT;
+			break;
+		case CLOCK_TAI:
+			tk_offset = TK_OFFS_TAI;
+			break;
+		default:
+			NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
+			return -EINVAL;
+		}
+	}
+
 	parm = nla_data(tb[TCA_GATE_PARMS]);
 	index = parm->index;
 
@@ -326,10 +368,6 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 		tcf_idr_release(*a, bind);
 		return -EEXIST;
 	}
-	if (ret == ACT_P_CREATED) {
-		to_gate(*a)->param.tcfg_clockid = -1;
-		INIT_LIST_HEAD(&(to_gate(*a)->param.entries));
-	}
 
 	if (tb[TCA_GATE_PRIORITY])
 		prio = nla_get_s32(tb[TCA_GATE_PRIORITY]);
@@ -340,33 +378,14 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	if (tb[TCA_GATE_FLAGS])
 		gflags = nla_get_u32(tb[TCA_GATE_FLAGS]);
 
-	if (tb[TCA_GATE_CLOCKID]) {
-		clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
-		switch (clockid) {
-		case CLOCK_REALTIME:
-			tk_offset = TK_OFFS_REAL;
-			break;
-		case CLOCK_MONOTONIC:
-			tk_offset = TK_OFFS_MAX;
-			break;
-		case CLOCK_BOOTTIME:
-			tk_offset = TK_OFFS_BOOT;
-			break;
-		case CLOCK_TAI:
-			tk_offset = TK_OFFS_TAI;
-			break;
-		default:
-			NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
-			goto release_idr;
-		}
-	}
+	gact = to_gate(*a);
+	if (ret == ACT_P_CREATED)
+		INIT_LIST_HEAD(&gact->param.entries);
 
 	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
 	if (err < 0)
 		goto release_idr;
 
-	gact = to_gate(*a);
-
 	spin_lock_bh(&gact->tcf_lock);
 	p = &gact->param;
 
@@ -397,14 +416,10 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 		p->tcfg_cycletime_ext =
 			nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
 
+	gate_setup_timer(gact, basetime, tk_offset, clockid,
+			 ret == ACT_P_CREATED);
 	p->tcfg_priority = prio;
-	p->tcfg_basetime = basetime;
-	p->tcfg_clockid = clockid;
 	p->tcfg_flags = gflags;
-
-	gact->tk_offset = tk_offset;
-	hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
-	gact->hitimer.function = gate_timer_func;
 	gate_get_start_time(gact, &start);
 
 	gact->current_close_time = start;
@@ -433,6 +448,13 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 release_idr:
+	/* action is not inserted in any list: it's safe to init hitimer
+	 * without taking tcf_lock.
+	 */
+	if (ret == ACT_P_CREATED)
+		gate_setup_timer(gact, gact->param.tcfg_basetime,
+				 gact->tk_offset, gact->param.tcfg_clockid,
+				 true);
 	tcf_idr_release(*a, bind);
 	return err;
 }
@@ -443,9 +465,7 @@ static void tcf_gate_cleanup(struct tc_action *a)
 	struct tcf_gate_params *p;
 
 	p = &gact->param;
-	if (p->tcfg_clockid != -1)
-		hrtimer_cancel(&gact->hitimer);
-
+	hrtimer_cancel(&gact->hitimer);
 	release_entry_list(&p->entries);
 }
 
-- 
2.26.2

Re: [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer

[Vladimir Oltean]

On Tue, 16 Jun 2020 at 23:25, Davide Caratti <dcaratti@redhat.com> wrote:
>
> assigning a dummy value of 'clock_id' to avoid cancellation of the cycle
> timer before its initialization was a temporary solution, and we still
> need to handle the case where act_gate timer parameters are changed by
> commands like the following one:
>
>  # tc action replace action gate <parameters>
>
> the fix consists in the following items:
>
> 1) remove the workaround assignment of 'clock_id', and init the list of
>    entries before the first error path after IDR atomic check/allocation
> 2) validate 'clock_id' earlier: there is no need to do IDR atomic
>    check/allocation if we know that 'clock_id' is a bad value
> 3) use a dedicated function, 'gate_setup_timer()', to ensure that the
>    timer is cancelled and re-initialized on action overwrite, and also
>    ensure we initialize the timer in the error path of tcf_gate_init()
>
> v3: improve comment in the error path of tcf_gate_init() (thanks to
>     Vladimir Oltean)
> v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)
>
> CC: Ivan Vecera <ivecera@redhat.com>
> Fixes: a01c245438c5 ("net/sched: fix a couple of splats in the error path of tfc_gate_init()")
> Fixes: a51c328df310 ("net: qos: introduce a gate control flow action")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  net/sched/act_gate.c | 90 +++++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 35 deletions(-)
>
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index 94e560c2f81d..323ae7f6315d 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> @@ -272,6 +272,27 @@ static int parse_gate_list(struct nlattr *list_attr,
>         return err;
>  }
>
> +static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> +                            enum tk_offsets tko, s32 clockid,
> +                            bool do_init)
> +{
> +       if (!do_init) {
> +               if (basetime == gact->param.tcfg_basetime &&
> +                   tko == gact->tk_offset &&
> +                   clockid == gact->param.tcfg_clockid)
> +                       return;
> +
> +               spin_unlock_bh(&gact->tcf_lock);
> +               hrtimer_cancel(&gact->hitimer);
> +               spin_lock_bh(&gact->tcf_lock);
> +       }
> +       gact->param.tcfg_basetime = basetime;
> +       gact->param.tcfg_clockid = clockid;
> +       gact->tk_offset = tko;
> +       hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
> +       gact->hitimer.function = gate_timer_func;
> +}
> +
>  static int tcf_gate_init(struct net *net, struct nlattr *nla,
>                          struct nlattr *est, struct tc_action **a,
>                          int ovr, int bind, bool rtnl_held,
> @@ -303,6 +324,27 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         if (!tb[TCA_GATE_PARMS])
>                 return -EINVAL;
>
> +       if (tb[TCA_GATE_CLOCKID]) {
> +               clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> +               switch (clockid) {
> +               case CLOCK_REALTIME:
> +                       tk_offset = TK_OFFS_REAL;
> +                       break;
> +               case CLOCK_MONOTONIC:
> +                       tk_offset = TK_OFFS_MAX;
> +                       break;
> +               case CLOCK_BOOTTIME:
> +                       tk_offset = TK_OFFS_BOOT;
> +                       break;
> +               case CLOCK_TAI:
> +                       tk_offset = TK_OFFS_TAI;
> +                       break;
> +               default:
> +                       NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> +                       return -EINVAL;
> +               }
> +       }
> +
>         parm = nla_data(tb[TCA_GATE_PARMS]);
>         index = parm->index;
>
> @@ -326,10 +368,6 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>                 tcf_idr_release(*a, bind);
>                 return -EEXIST;
>         }
> -       if (ret == ACT_P_CREATED) {
> -               to_gate(*a)->param.tcfg_clockid = -1;
> -               INIT_LIST_HEAD(&(to_gate(*a)->param.entries));
> -       }
>
>         if (tb[TCA_GATE_PRIORITY])
>                 prio = nla_get_s32(tb[TCA_GATE_PRIORITY]);
> @@ -340,33 +378,14 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         if (tb[TCA_GATE_FLAGS])
>                 gflags = nla_get_u32(tb[TCA_GATE_FLAGS]);
>
> -       if (tb[TCA_GATE_CLOCKID]) {
> -               clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> -               switch (clockid) {
> -               case CLOCK_REALTIME:
> -                       tk_offset = TK_OFFS_REAL;
> -                       break;
> -               case CLOCK_MONOTONIC:
> -                       tk_offset = TK_OFFS_MAX;
> -                       break;
> -               case CLOCK_BOOTTIME:
> -                       tk_offset = TK_OFFS_BOOT;
> -                       break;
> -               case CLOCK_TAI:
> -                       tk_offset = TK_OFFS_TAI;
> -                       break;
> -               default:
> -                       NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> -                       goto release_idr;
> -               }
> -       }
> +       gact = to_gate(*a);
> +       if (ret == ACT_P_CREATED)
> +               INIT_LIST_HEAD(&gact->param.entries);
>
>         err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
>         if (err < 0)
>                 goto release_idr;
>
> -       gact = to_gate(*a);
> -
>         spin_lock_bh(&gact->tcf_lock);
>         p = &gact->param;
>
> @@ -397,14 +416,10 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>                 p->tcfg_cycletime_ext =
>                         nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
>
> +       gate_setup_timer(gact, basetime, tk_offset, clockid,
> +                        ret == ACT_P_CREATED);
>         p->tcfg_priority = prio;
> -       p->tcfg_basetime = basetime;
> -       p->tcfg_clockid = clockid;
>         p->tcfg_flags = gflags;
> -
> -       gact->tk_offset = tk_offset;
> -       hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
> -       gact->hitimer.function = gate_timer_func;
>         gate_get_start_time(gact, &start);
>
>         gact->current_close_time = start;
> @@ -433,6 +448,13 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
>  release_idr:
> +       /* action is not inserted in any list: it's safe to init hitimer
> +        * without taking tcf_lock.
> +        */
> +       if (ret == ACT_P_CREATED)
> +               gate_setup_timer(gact, gact->param.tcfg_basetime,
> +                                gact->tk_offset, gact->param.tcfg_clockid,
> +                                true);
>         tcf_idr_release(*a, bind);
>         return err;
>  }
> @@ -443,9 +465,7 @@ static void tcf_gate_cleanup(struct tc_action *a)
>         struct tcf_gate_params *p;
>
>         p = &gact->param;
> -       if (p->tcfg_clockid != -1)
> -               hrtimer_cancel(&gact->hitimer);
> -
> +       hrtimer_cancel(&gact->hitimer);
>         release_entry_list(&p->entries);
>  }
>
> --
> 2.26.2
>

Re: [PATCH net v3 0/2] two fixes for 'act_gate' control plane

[David Miller]

From: Davide Caratti <dcaratti@redhat.com>
Date: Tue, 16 Jun 2020 22:25:19 +0200

> - patch 1/2 attempts to fix the error path of tcf_gate_init() when users
>   try to configure 'act_gate' rules with wrong parameters
> - patch 2/2 is a follow-up of a recent fix for NULL dereference in
>   the error path of tcf_gate_init()
> 
> further work will introduce a tdc test for 'act_gate'.
> 
> changes since v2:
>   - fix undefined behavior in patch 1/2
>   - improve comment in patch 2/2
> changes since v1:
>   coding style fixes in patch 1/2 and 2/2

Series applied, thanks Davide.