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

[PATCH net v2 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 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 | 124 +++++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 58 deletions(-)

-- 
2.26.2

[PATCH net v2 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.

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..6775ccf355b0 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, 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 v2 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init()

[Vladimir Oltean]

Hi Davide,

On Mon, 15 Jun 2020 at 22:31, 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.
>
> 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..6775ccf355b0 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, basetime = 0;

You must initialize cycletime with zero here...

>         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) {

...otherwise it won't enter this 'if' block here.

>                 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
>

Cheers,
-Vladimir

[PATCH net v2 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()

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 | 88 ++++++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 35 deletions(-)

diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 6775ccf355b0..3c529a4bcca5 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,11 @@ 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 in: hitimer can be inited 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 +463,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 v2 2/2] net/sched: act_gate: fix configuration of the periodic timer

[Vladimir Oltean]

On Mon, 15 Jun 2020 at 22:33, 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()
>
> v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)
>

The change log is put under the 3 '---' characters for a reason: it is
relevant only to reviewers, and git automatically trims it when
applying the patch. The way it is now, the commit message would
contain this line about "v2 ...".

> 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 | 88 ++++++++++++++++++++++++++------------------
>  1 file changed, 53 insertions(+), 35 deletions(-)
>
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index 6775ccf355b0..3c529a4bcca5 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);

I think it's horrible to do this just to get out of atomic context.
What if you split the "replace" functionality of gate_setup_timer into
a separate gate_cancel_timer function, which you could call earlier
(before taking the spin lock)? That change would look like this:

diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 3c529a4bcca5..47c625a0e70c 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -273,19 +273,8 @@ static int parse_gate_list(struct nlattr *list_attr,
 }

 static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
-                 enum tk_offsets tko, s32 clockid,
-                 bool do_init)
+                 enum tk_offsets tko, s32 clockid)
 {
-    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;
@@ -293,6 +282,17 @@ static void gate_setup_timer(struct tcf_gate
*gact, u64 basetime,
     gact->hitimer.function = gate_timer_func;
 }

+static void gate_cancel_timer(struct tcf_gate *gact, u64 basetime,
+                  enum tk_offsets tko, s32 clockid)
+{
+    if (basetime == gact->param.tcfg_basetime &&
+        tko == gact->tk_offset &&
+        clockid == gact->param.tcfg_clockid)
+        return;
+
+    hrtimer_cancel(&gact->hitimer);
+}
+
 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,
@@ -381,6 +381,8 @@ static int tcf_gate_init(struct net *net, struct
nlattr *nla,
     gact = to_gate(*a);
     if (ret == ACT_P_CREATED)
         INIT_LIST_HEAD(&gact->param.entries);
+    else
+        gate_cancel_timer(gact, basetime, tk_offset, clockid);

     err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
     if (err < 0)
@@ -416,8 +418,7 @@ 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);
+    gate_setup_timer(gact, basetime, tk_offset, clockid);
     p->tcfg_priority = prio;
     p->tcfg_flags = gflags;
     gate_get_start_time(gact, &start);
@@ -451,8 +452,7 @@ static int tcf_gate_init(struct net *net, struct
nlattr *nla,
     /* action is not in: hitimer can be inited 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);
+                 gact->tk_offset, gact->param.tcfg_clockid);
     tcf_idr_release(*a, bind);
     return err;
 }


> +       }
> +       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,11 @@ 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 in: hitimer can be inited 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 +463,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
>

Thanks,
-Vladimir

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

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 16 Jun 2020 00:55:50 +0300

> On Mon, 15 Jun 2020 at 22:33, 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()
>>
>> v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)
>>
> 
> The change log is put under the 3 '---' characters for a reason: it is
> relevant only to reviewers, and git automatically trims it when
> applying the patch. The way it is now, the commit message would
> contain this line about "v2 ...".

I completely disagree and I ask submitters of networking changes to keep
the changelog in the commit message.

Later people will look at this commit and ask "why didn't they do X
or Y" and if the changelog shows that the submitter was asked not to
do X or Y that is useful information.

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

[Vladimir Oltean]

On Tue, 16 Jun 2020 at 04:00, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Tue, 16 Jun 2020 00:55:50 +0300
>
> > On Mon, 15 Jun 2020 at 22:33, 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()
> >>
> >> v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)
> >>
> >
> > The change log is put under the 3 '---' characters for a reason: it is
> > relevant only to reviewers, and git automatically trims it when
> > applying the patch. The way it is now, the commit message would
> > contain this line about "v2 ...".
>
> I completely disagree and I ask submitters of networking changes to keep
> the changelog in the commit message.
>
> Later people will look at this commit and ask "why didn't they do X
> or Y" and if the changelog shows that the submitter was asked not to
> do X or Y that is useful information.

Interesting, I didn't know that. If there are really relevant changes
which might matter post-review (which I don't consider "avoid 'goto'
in gate_setup_timer" to be), I usually try to integrate them better
into the main commit message. Nonetheless, sorry, feel free to ignore
me!

-Vladimir

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

[Davide Caratti]

hello Vladimir,

thanks a lot for reviewing this.

On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:

[...]

> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > index 6775ccf355b0..3c529a4bcca5 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);
> 
> I think it's horrible to do this just to get out of atomic context.
> What if you split the "replace" functionality of gate_setup_timer into
> a separate gate_cancel_timer function, which you could call earlier
> (before taking the spin lock)? 

I think it would introduce the following 2 problems:

problem #1) a race condition, see below:

> That change would look like this:
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index 3c529a4bcca5..47c625a0e70c 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> @@ -273,19 +273,8 @@ static int parse_gate_list(struct nlattr *list_attr,
>  }
> 
>  static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> -                 enum tk_offsets tko, s32 clockid,
> -                 bool do_init)
> +                 enum tk_offsets tko, s32 clockid)
>  {
> -    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;
> @@ -293,6 +282,17 @@ static void gate_setup_timer(struct tcf_gate
> *gact, u64 basetime,
>      gact->hitimer.function = gate_timer_func;
>  }
> 
> +static void gate_cancel_timer(struct tcf_gate *gact, u64 basetime,
> +                  enum tk_offsets tko, s32 clockid)
> +{
> +    if (basetime == gact->param.tcfg_basetime &&
> +        tko == gact->tk_offset &&
> +        clockid == gact->param.tcfg_clockid)
> +        return;
> +
> +    hrtimer_cancel(&gact->hitimer);
> +}
> +

the above function either cancels a timer, or does nothing: it depends on
the value of the 3-ple {tcfg_basetime, tk_offset, tcfg_clockid}. If we run
this function without holding tcf_lock, nobody will guarantee that
{tcfg_basetime, tk_offset, tcfg_clockid} is not being concurrently
rewritten by some other command like:

# tc action replace action gate <parameters> index <x>

>  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,
> @@ -381,6 +381,8 @@ static int tcf_gate_init(struct net *net, struct
> nlattr *nla,
>      gact = to_gate(*a);
>      if (ret == ACT_P_CREATED)
>          INIT_LIST_HEAD(&gact->param.entries);
> +    else
> +        gate_cancel_timer(gact, basetime, tk_offset, clockid);
> 

IOW, the above line is racy unless we do spin_lock()/spin_unlock() around
the

if (<expression depending on gact-> members>)
	return; 

statement before hrtimer_cancel(), which does not seem much different
than what I did in gate_setup_timer().

[...]

> @@ -433,6 +448,11 @@ 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 in: hitimer can be inited 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);

please note, here I felt the need to add a comment, because when ret ==
ACT_P_CREATED the action is not inserted in any list, so there is no
concurrent writer of gact-> members for that action.

> >         tcf_idr_release(*a, bind);
> >         return err;
> >  }

problem #2) a functional issue that originates in how 'cycle_time' and
'entries' are validated (*). See below:

On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:

> 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,
> @@ -381,6 +381,8 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>      gact = to_gate(*a);
>      if (ret == ACT_P_CREATED)
>          INIT_LIST_HEAD(&gact->param.entries);
> +    else
> +        gate_cancel_timer(gact, basetime, tk_offset, clockid);

here you propose to cancel the timer, but few lines after we have this:

385         err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
386         if (err < 0)
387                 goto release_idr;
388 

so, when users try the following commands:

# tc action add action gate <good parameters> index 2
# tc action replace action gate <other good parameters> goto chain 42 index 2

and chain 42 does not exist, the second command will fail. But the timer
is erroneously stopped, and never started again. So, the first rule is
correctly inserted but it becomes no more functional after users try to
replace it with another one having invalid control action.

Moving the call to gate_cancel_timer() after the validation of the control
action will not fix this problem, because 'cycle_time' and 'entries' are
validated together, and with the spinlock taken. Because of this, we need
to cancel that timer only when we know that we will not do
tcf_idr_release() and return some error to the user.

please let me know if you think my doubts are not well-founded.

-- 
davide

(*) now that I see parse_gate_list() again, I noticed another potential
issue with replace (that I need to verify first): apparently the list is
not replaced, it's just "updated" with new entries appended at the end. I
will try to write a fix for that (separate from this series).

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

[Vladimir Oltean]

Hi Davide,

On Tue, 16 Jun 2020 at 13:13, Davide Caratti <dcaratti@redhat.com> wrote:
>
> hello Vladimir,
>
> thanks a lot for reviewing this.
>
> On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:
>
> [...]
>
> > > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > > index 6775ccf355b0..3c529a4bcca5 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);
> >
> > I think it's horrible to do this just to get out of atomic context.
> > What if you split the "replace" functionality of gate_setup_timer into
> > a separate gate_cancel_timer function, which you could call earlier
> > (before taking the spin lock)?
>
> I think it would introduce the following 2 problems:
>
> problem #1) a race condition, see below:
>

I must have been living under a stone since I missed the entire
unlocked tc filter rework done by Vlad Buslov, I thought that
tcf_action_init always runs under rtnl. So it is clear now.

> > That change would look like this:
> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > index 3c529a4bcca5..47c625a0e70c 100644
> > --- a/net/sched/act_gate.c
> > +++ b/net/sched/act_gate.c
> > @@ -273,19 +273,8 @@ static int parse_gate_list(struct nlattr *list_attr,
> >  }
> >
> >  static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> > -                 enum tk_offsets tko, s32 clockid,
> > -                 bool do_init)
> > +                 enum tk_offsets tko, s32 clockid)
> >  {
> > -    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;
> > @@ -293,6 +282,17 @@ static void gate_setup_timer(struct tcf_gate
> > *gact, u64 basetime,
> >      gact->hitimer.function = gate_timer_func;
> >  }
> >
> > +static void gate_cancel_timer(struct tcf_gate *gact, u64 basetime,
> > +                  enum tk_offsets tko, s32 clockid)
> > +{
> > +    if (basetime == gact->param.tcfg_basetime &&
> > +        tko == gact->tk_offset &&
> > +        clockid == gact->param.tcfg_clockid)
> > +        return;
> > +
> > +    hrtimer_cancel(&gact->hitimer);
> > +}
> > +
>
> the above function either cancels a timer, or does nothing: it depends on
> the value of the 3-ple {tcfg_basetime, tk_offset, tcfg_clockid}. If we run
> this function without holding tcf_lock, nobody will guarantee that
> {tcfg_basetime, tk_offset, tcfg_clockid} is not being concurrently
> rewritten by some other command like:
>
> # tc action replace action gate <parameters> index <x>
>
> >  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,
> > @@ -381,6 +381,8 @@ static int tcf_gate_init(struct net *net, struct
> > nlattr *nla,
> >      gact = to_gate(*a);
> >      if (ret == ACT_P_CREATED)
> >          INIT_LIST_HEAD(&gact->param.entries);
> > +    else
> > +        gate_cancel_timer(gact, basetime, tk_offset, clockid);
> >
>
> IOW, the above line is racy unless we do spin_lock()/spin_unlock() around
> the
>
> if (<expression depending on gact-> members>)
>         return;
>
> statement before hrtimer_cancel(), which does not seem much different
> than what I did in gate_setup_timer().
>
> [...]
>
> > @@ -433,6 +448,11 @@ 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 in: hitimer can be inited 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);
>
> please note, here I felt the need to add a comment, because when ret ==
> ACT_P_CREATED the action is not inserted in any list, so there is no
> concurrent writer of gact-> members for that action.
>

Then please rephrase the comment. I had read it and it still wasn't
clear at all for me what you were talking about.

> > >         tcf_idr_release(*a, bind);
> > >         return err;
> > >  }
>
> problem #2) a functional issue that originates in how 'cycle_time' and
> 'entries' are validated (*). See below:
>
> On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:
>
> > 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,
> > @@ -381,6 +381,8 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> >      gact = to_gate(*a);
> >      if (ret == ACT_P_CREATED)
> >          INIT_LIST_HEAD(&gact->param.entries);
> > +    else
> > +        gate_cancel_timer(gact, basetime, tk_offset, clockid);
>
> here you propose to cancel the timer, but few lines after we have this:
>
> 385         err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> 386         if (err < 0)
> 387                 goto release_idr;
> 388
>
> so, when users try the following commands:
>
> # tc action add action gate <good parameters> index 2
> # tc action replace action gate <other good parameters> goto chain 42 index 2
>
> and chain 42 does not exist, the second command will fail. But the timer
> is erroneously stopped, and never started again. So, the first rule is
> correctly inserted but it becomes no more functional after users try to
> replace it with another one having invalid control action.
>

Yes, correct.

> Moving the call to gate_cancel_timer() after the validation of the control
> action will not fix this problem, because 'cycle_time' and 'entries' are
> validated together, and with the spinlock taken. Because of this, we need
> to cancel that timer only when we know that we will not do
> tcf_idr_release() and return some error to the user.
>
> please let me know if you think my doubts are not well-founded.
>
> --
> davide
>
> (*) now that I see parse_gate_list() again, I noticed another potential
> issue with replace (that I need to verify first): apparently the list is
> not replaced, it's just "updated" with new entries appended at the end. I
> will try to write a fix for that (separate from this series).
>
>

I wonder, could you call tcf_gate_cleanup instead of just canceling the hrtimer?

Thanks,
-Vladimir

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

[Davide Caratti]

On Tue, 2020-06-16 at 13:38 +0300, Vladimir Oltean wrote:
> Hi Davide,
> 
> On Tue, 16 Jun 2020 at 13:13, Davide Caratti <dcaratti@redhat.com> wrote:
> > hello Vladimir,
> > 
> > thanks a lot for reviewing this.
> > 
> > On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:

[...]

> > > What if you split the "replace" functionality of gate_setup_timer into
> > > a separate gate_cancel_timer function, which you could call earlier
> > > (before taking the spin lock)?
> > 
> > I think it would introduce the following 2 problems:
> >
> > problem #1) a race condition, see below:

[...]

> > > @@ -433,6 +448,11 @@ 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 in: hitimer can be inited 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);
> > 
> > please note, here I felt the need to add a comment, because when ret ==
> > ACT_P_CREATED the action is not inserted in any list, so there is no
> > concurrent writer of gact-> members for that action.
> > 
> 
> Then please rephrase the comment. I had read it and it still wasn't
> clear at all for me what you were talking about.

something like:

/* action is not yet inserted in any list: it's safe to init hitimer 
 * without taking tcf_lock.
 */

would be ok?

[...]

> I wonder, could you call tcf_gate_cleanup instead of just canceling the
> hrtimer?

not with the current tcf_gate_cleanup() [1] and parse_gate_list() [2],
because it would introduce another bug: 'p->entries' gets cleared on
action overwrite after being successfully created here:

395         if (tb[TCA_GATE_ENTRY_LIST]) {
396                 err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
397                 if (err < 0)
398                         goto chain_put;
399         }


like mentioned earlier, 'hitimer' can not be canceled/re-initialized easily when
tcf_gate_init() still has a possible error path. And in my understanding
'p->entries' must be consistent when the timer is initialized.

IMO, the correct way to handle 'entries' is to:

- populate the list on a local variable, before taking the spinlock and
allocating the IDR

- assign to p->entries after validation is successful (with the spinlock
taken). Same as what was done with 'cycletime' in patch 1/2, but with the
variable initialized (btw, thanks for catching this), and free the old
list in case of action replace

- release the newly allocated list in the error path of tcf_gate_init()

(but again, this would be a fix for 'entries' - not for 'hitimer', so I
plan to work on it as a separate patch, that fits better 'net-next' rather
than 'net').

-- 
davide

[1] https://elixir.bootlin.com/linux/v5.8-rc1/source/net/sched/act_gate.c#L450
[2] https://elixir.bootlin.com/linux/v5.8-rc1/source/net/sched/act_gate.c#L235

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

[Vladimir Oltean]

On Tue, 16 Jun 2020 at 15:43, Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Tue, 2020-06-16 at 13:38 +0300, Vladimir Oltean wrote:
> > Hi Davide,
> >
> > On Tue, 16 Jun 2020 at 13:13, Davide Caratti <dcaratti@redhat.com> wrote:
> > > hello Vladimir,
> > >
> > > thanks a lot for reviewing this.
> > >
> > > On Tue, 2020-06-16 at 00:55 +0300, Vladimir Oltean wrote:
>
> [...]
>
> > > > What if you split the "replace" functionality of gate_setup_timer into
> > > > a separate gate_cancel_timer function, which you could call earlier
> > > > (before taking the spin lock)?
> > >
> > > I think it would introduce the following 2 problems:
> > >
> > > problem #1) a race condition, see below:
>
> [...]
>
> > > > @@ -433,6 +448,11 @@ 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 in: hitimer can be inited 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);
> > >
> > > please note, here I felt the need to add a comment, because when ret ==
> > > ACT_P_CREATED the action is not inserted in any list, so there is no
> > > concurrent writer of gact-> members for that action.
> > >
> >
> > Then please rephrase the comment. I had read it and it still wasn't
> > clear at all for me what you were talking about.
>
> something like:
>
> /* action is not yet inserted in any list: it's safe to init hitimer
>  * without taking tcf_lock.
>  */
>
> would be ok?
>

Yes, better.

> [...]
>
> > I wonder, could you call tcf_gate_cleanup instead of just canceling the
> > hrtimer?
>
> not with the current tcf_gate_cleanup() [1] and parse_gate_list() [2],
> because it would introduce another bug: 'p->entries' gets cleared on
> action overwrite after being successfully created here:
>
> 395         if (tb[TCA_GATE_ENTRY_LIST]) {
> 396                 err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
> 397                 if (err < 0)
> 398                         goto chain_put;
> 399         }
>
>
> like mentioned earlier, 'hitimer' can not be canceled/re-initialized easily when
> tcf_gate_init() still has a possible error path. And in my understanding
> 'p->entries' must be consistent when the timer is initialized.
>
> IMO, the correct way to handle 'entries' is to:
>
> - populate the list on a local variable, before taking the spinlock and
> allocating the IDR
>
> - assign to p->entries after validation is successful (with the spinlock
> taken). Same as what was done with 'cycletime' in patch 1/2, but with the
> variable initialized (btw, thanks for catching this), and free the old
> list in case of action replace
>
> - release the newly allocated list in the error path of tcf_gate_init()
>
> (but again, this would be a fix for 'entries' - not for 'hitimer', so I
> plan to work on it as a separate patch, that fits better 'net-next' rather
> than 'net').
>

Targeting net-next would mean that the net tree would still keep
appending to p->entries upon action replacement, instead of just
replacing p->entries?

> --
> davide
>
> [1] https://elixir.bootlin.com/linux/v5.8-rc1/source/net/sched/act_gate.c#L450
> [2] https://elixir.bootlin.com/linux/v5.8-rc1/source/net/sched/act_gate.c#L235
>

Thanks,
-Vladimir

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

[Davide Caratti]

On Tue, 2020-06-16 at 17:23 +0300, Vladimir Oltean wrote:
> > (but again, this would be a fix for 'entries' - not for 'hitimer', so I
> > plan to work on it as a separate patch, that fits better 'net-next' rather
> > than 'net').
> 
> Targeting net-next would mean that the net tree would still keep
> appending to p->entries upon action replacement, instead of just
> replacing p->entries?

well, this is the original act_gate behavior (and the bug is discovered
today, in this thread). But if users can't wait for the proper fix (and
equally important, for the tdc test file), I can think of sending the
patch directly for 'net'.

-- 
davide

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

[Vladimir Oltean]

On Tue, 16 Jun 2020 at 17:58, Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Tue, 2020-06-16 at 17:23 +0300, Vladimir Oltean wrote:
> > > (but again, this would be a fix for 'entries' - not for 'hitimer', so I
> > > plan to work on it as a separate patch, that fits better 'net-next' rather
> > > than 'net').
> >
> > Targeting net-next would mean that the net tree would still keep
> > appending to p->entries upon action replacement, instead of just
> > replacing p->entries?
>
> well, this is the original act_gate behavior (and the bug is discovered
> today, in this thread). But if users can't wait for the proper fix (and
> equally important, for the tdc test file), I can think of sending the
> patch directly for 'net'.
>
> --
> davide
>

Well, it's up to you. Considering that your fixes here are for the 'tc
action replace' case, I guess you might as well go all the way and
make it work properly :)

Cheers,
-Vladimir