| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+h21hqEnC_oYDqvGhJnTszHXKdBYb_PLzd7fFx7K=if6KKQiw@mail.gmail.com>
Date: Wed, 17 Jun 2020 01:52:58 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Davide Caratti <dcaratti@...hat.com>
Cc: Po Liu <Po.Liu@....com>, Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v3 2/2] net/sched: act_gate: fix configuration of the
periodic timer
On Tue, 16 Jun 2020 at 23:25, Davide Caratti <dcaratti@...hat.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@...hat.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@...hat.com>
> ---
Acked-by: Vladimir Oltean <vladimir.oltean@....com>
Tested-by: Vladimir Oltean <vladimir.oltean@....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
>
Powered by blists - more mailing lists