* [PATCH net v3 0/2] two fixes for 'act_gate' control plane
@ 2020-06-16 20:25 Davide Caratti
2020-06-16 20:25 ` [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init() Davide Caratti
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Davide Caratti @ 2020-06-16 20:25 UTC (permalink / raw)
To: Po Liu, Cong Wang, Vladimir Oltean; +Cc: David S. Miller, netdev
- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init()
2020-06-16 20:25 [PATCH net v3 0/2] two fixes for 'act_gate' control plane Davide Caratti
@ 2020-06-16 20:25 ` Davide Caratti
2020-06-16 22:51 ` Vladimir Oltean
2020-06-16 20:25 ` [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer Davide Caratti
2020-06-19 3:18 ` [PATCH net v3 0/2] two fixes for 'act_gate' control plane David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2020-06-16 20:25 UTC (permalink / raw)
To: Po Liu, Cong Wang, Vladimir Oltean; +Cc: David S. Miller, netdev
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer
2020-06-16 20:25 [PATCH net v3 0/2] two fixes for 'act_gate' control plane Davide Caratti
2020-06-16 20:25 ` [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init() Davide Caratti
@ 2020-06-16 20:25 ` Davide Caratti
2020-06-16 22:52 ` Vladimir Oltean
2020-06-19 3:18 ` [PATCH net v3 0/2] two fixes for 'act_gate' control plane David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Davide Caratti @ 2020-06-16 20:25 UTC (permalink / raw)
To: Po Liu, Cong Wang, Vladimir Oltean; +Cc: David S. Miller, netdev
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init()
2020-06-16 20:25 ` [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init() Davide Caratti
@ 2020-06-16 22:51 ` Vladimir Oltean
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2020-06-16 22:51 UTC (permalink / raw)
To: Davide Caratti; +Cc: Po Liu, Cong Wang, David S. Miller, netdev
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer
2020-06-16 20:25 ` [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer Davide Caratti
@ 2020-06-16 22:52 ` Vladimir Oltean
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2020-06-16 22:52 UTC (permalink / raw)
To: Davide Caratti; +Cc: Po Liu, Cong Wang, David S. Miller, netdev
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3 0/2] two fixes for 'act_gate' control plane
2020-06-16 20:25 [PATCH net v3 0/2] two fixes for 'act_gate' control plane Davide Caratti
2020-06-16 20:25 ` [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init() Davide Caratti
2020-06-16 20:25 ` [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer Davide Caratti
@ 2020-06-19 3:18 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-06-19 3:18 UTC (permalink / raw)
To: dcaratti; +Cc: Po.Liu, xiyou.wangcong, olteanv, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-19 3:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-16 20:25 [PATCH net v3 0/2] two fixes for 'act_gate' control plane Davide Caratti
2020-06-16 20:25 ` [PATCH net v3 1/2] net/sched: act_gate: fix NULL dereference in tcf_gate_init() Davide Caratti
2020-06-16 22:51 ` Vladimir Oltean
2020-06-16 20:25 ` [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the periodic timer Davide Caratti
2020-06-16 22:52 ` Vladimir Oltean
2020-06-19 3:18 ` [PATCH net v3 0/2] two fixes for 'act_gate' control plane David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).