* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.