| 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: <VE1PR04MB649621D4ADDD23C899BC476C92FB0@VE1PR04MB6496.eurprd04.prod.outlook.com>
Date: Sat, 14 Mar 2020 04:09:04 +0000
From: Po Liu <po.liu@....com>
To: Cong Wang <xiyou.wangcong@...il.com>
CC: David Miller <davem@...emloft.net>,
LKML <linux-kernel@...r.kernel.org>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
Vinicius Costa Gomes <vinicius.gomes@...el.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Alexandru Marginean <alexandru.marginean@....com>,
Xiaoliang Yang <xiaoliang.yang_1@....com>,
Roy Zang <roy.zang@....com>, Mingkai Hu <mingkai.hu@....com>,
Jerry Huang <jerry.huang@....com>, Leo Li <leoyang.li@....com>,
"michael.chan@...adcom.com" <michael.chan@...adcom.com>,
"vishal@...lsio.com" <vishal@...lsio.com>,
Saeed Mahameed <saeedm@...lanox.com>,
Leon Romanovsky <leon@...nel.org>,
Jiri Pirko <jiri@...lanox.com>,
Ido Schimmel <idosch@...lanox.com>,
"alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
Jakub Kicinski <kuba@...nel.org>,
Jamal Hadi Salim <jhs@...atatu.com>,
Simon Horman <simon.horman@...ronome.com>,
Pablo Neira Ayuso <pablo@...filter.org>,
"moshe@...lanox.com" <moshe@...lanox.com>,
Murali Karicheri <m-karicheri2@...com>,
"andre.guedes@...ux.intel.com" <andre.guedes@...ux.intel.com>
Subject: RE: [EXT] Re: [RFC,net-next 2/9] net: qos: introduce a gate control
flow action
Hi Cong,
> -----Original Message-----
> From: Cong Wang <xiyou.wangcong@...il.com>
> Sent: 2020年3月14日 2:37
> To: Po Liu <po.liu@....com>
> Cc: David Miller <davem@...emloft.net>; LKML <linux-
> kernel@...r.kernel.org>; Linux Kernel Network Developers
> <netdev@...r.kernel.org>; Vinicius Costa Gomes
> <vinicius.gomes@...el.com>; Claudiu Manoil <claudiu.manoil@....com>;
> Vladimir Oltean <vladimir.oltean@....com>; Alexandru Marginean
> <alexandru.marginean@....com>; Xiaoliang Yang
> <xiaoliang.yang_1@....com>; Roy Zang <roy.zang@....com>; Mingkai Hu
> <mingkai.hu@....com>; Jerry Huang <jerry.huang@....com>; Leo Li
> <leoyang.li@....com>; michael.chan@...adcom.com; vishal@...lsio.com;
> Saeed Mahameed <saeedm@...lanox.com>; Leon Romanovsky
> <leon@...nel.org>; Jiri Pirko <jiri@...lanox.com>; Ido Schimmel
> <idosch@...lanox.com>; alexandre.belloni@...tlin.com;
> UNGLinuxDriver@...rochip.com; Jakub Kicinski <kuba@...nel.org>; Jamal
> Hadi Salim <jhs@...atatu.com>; John Hurley
> <john.hurley@...ronome.com>; Simon Horman
> <simon.horman@...ronome.com>; Pieter Jansen van Vuuren
> <pieter.jansenvanvuuren@...ronome.com>; Pablo Neira Ayuso
> <pablo@...filter.org>; moshe@...lanox.com;
> ivan.khoronzhuk@...aro.org; Murali Karicheri <m-karicheri2@...com>;
> andre.guedes@...ux.intel.com; Jakub Kicinski
> <jakub.kicinski@...ronome.com>
> Subject: [EXT] Re: [RFC,net-next 2/9] net: qos: introduce a gate control flow
> action
>
> Caution: EXT Email
>
> On Fri, Mar 6, 2020 at 5:14 AM Po Liu <Po.Liu@....com> wrote:
> > +static int parse_gate_list(struct nlattr *list,
> > + struct tcf_gate_params *sched,
> > + struct netlink_ext_ack *extack) {
> > + struct nlattr *n;
> > + int err, rem;
> > + int i = 0;
> > +
> > + if (!list)
> > + return -EINVAL;
> > +
> > + nla_for_each_nested(n, list, rem) {
> > + struct tcfg_gate_entry *entry;
> > +
> > + if (nla_type(n) != TCA_GATE_ONE_ENTRY) {
> > + NL_SET_ERR_MSG(extack, "Attribute isn't type 'entry'");
> > + continue;
> > + }
> > +
> > + entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > + if (!entry) {
> > + NL_SET_ERR_MSG(extack, "Not enough memory for
> entry");
> > + return -ENOMEM;
> > + }
>
> You need to free all previous allocated entries on this list when an error
> happens, right?
Yes, just as Vinicius has mentioned. I'll fix it next version.
>
>
> > +
> > + err = parse_gate_entry(n, entry, i, extack);
> > + if (err < 0) {
> > + kfree(entry);
> > + return err;
> > + }
> > +
> > + list_add_tail(&entry->list, &sched->entries);
> > + i++;
> > + }
> > +
> > + sched->num_entries = i;
> > +
> > + return i;
> > +}
> > +
> > +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,
> > + struct tcf_proto *tp, u32 flags,
> > + struct netlink_ext_ack *extack) {
> > + struct tc_action_net *tn = net_generic(net, gate_net_id);
> > + enum tk_offsets tk_offset = TK_OFFS_TAI;
> > + struct nlattr *tb[TCA_GATE_MAX + 1];
> > + struct tcf_chain *goto_ch = NULL;
> > + struct tcfg_gate_entry *next;
> > + struct tcf_gate_params *p;
> > + struct gate_action *gact;
> > + s32 clockid = CLOCK_TAI;
> > + struct tc_gate *parm;
> > + struct tcf_gate *g;
> > + int ret = 0, err;
> > + u64 basetime = 0;
> > + u32 gflags = 0;
> > + s32 prio = -1;
> > + ktime_t start;
> > + u32 index;
> > +
> > + if (!nla)
> > + return -EINVAL;
> > +
> > + err = nla_parse_nested_deprecated(tb, TCA_GATE_MAX,
> > + nla, gate_policy, NULL);
> > + if (err < 0)
> > + return err;
> > +
> > + if (!tb[TCA_GATE_PARMS])
> > + return -EINVAL;
> > + parm = nla_data(tb[TCA_GATE_PARMS]);
> > + index = parm->index;
> > + err = tcf_idr_check_alloc(tn, &index, a, bind);
> > + if (err < 0)
> > + return err;
> > +
> > + if (err && bind)
> > + return 0;
> > +
> > + if (!err) {
> > + ret = tcf_idr_create_from_flags(tn, index, est, a,
> > + &act_gate_ops, bind, flags);
> > + if (ret) {
> > + tcf_idr_cleanup(tn, index);
> > + return ret;
> > + }
> > +
> > + ret = ACT_P_CREATED;
> > + } else if (!ovr) {
> > + tcf_idr_release(*a, bind);
> > + return -EEXIST;
> > + }
> > +
> > + if (tb[TCA_GATE_PRIORITY])
> > + prio = nla_get_s32(tb[TCA_GATE_PRIORITY]);
> > +
> > + if (tb[TCA_GATE_BASE_TIME])
> > + basetime = nla_get_u64(tb[TCA_GATE_BASE_TIME]);
> > +
> > + 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;
> > + }
> > + }
> > +
> > + err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> > + if (err < 0)
> > + goto release_idr;
> > +
> > + g = to_gate(*a);
> > +
> > + gact = kzalloc(sizeof(*gact), GFP_KERNEL);
> > + if (!gact) {
> > + err = -ENOMEM;
> > + goto put_chain;
> > + }
> > +
> > + p = get_gate_param(gact);
> > +
> > + INIT_LIST_HEAD(&p->entries);
> > + if (tb[TCA_GATE_ENTRY_LIST]) {
> > + err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
> > + if (err < 0)
> > + goto release_mem;
> > + }
> > +
> > + if (tb[TCA_GATE_CYCLE_TIME]) {
> > + p->tcfg_cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
> > + } else {
> > + 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;
> > + }
> > +
> > + if (tb[TCA_GATE_CYCLE_TIME_EXT])
> > + p->tcfg_cycletime_ext =
> > + nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
> > +
> > + p->tcfg_priority = prio;
> > + p->tcfg_basetime = basetime;
> > + p->tcfg_clockid = clockid;
> > + p->tcfg_flags = gflags;
> > +
> > + gact->tk_offset = tk_offset;
> > + spin_lock_init(&gact->entry_lock);
> > + hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS);
> > + 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");
> > + goto release_mem;
> > + }
> > +
> > + gact->current_close_time = start;
> > + gact->current_gate_status = GATE_ACT_GATE_OPEN |
> > + GATE_ACT_PENDING;
> > +
> > + next = list_first_entry(&p->entries, struct tcfg_gate_entry, list);
> > + rcu_assign_pointer(gact->next_entry, next);
> > +
> > + gate_start_timer(gact, start);
> > +
> > + spin_lock_bh(&g->tcf_lock);
> > + goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > + gact = rcu_replace_pointer(g->actg, gact,
> > + lockdep_is_held(&g->tcf_lock));
> > + spin_unlock_bh(&g->tcf_lock);
> > +
> > + if (goto_ch)
> > + tcf_chain_put_by_act(goto_ch);
> > + if (gact)
> > + kfree_rcu(gact, rcu);
> > +
> > + if (ret == ACT_P_CREATED)
> > + tcf_idr_insert(tn, *a);
> > + return ret;
> > +
> > +release_mem:
> > + kfree_rcu(gact, rcu);
>
> No need to bother RCU for an error path, right?
Yes, the kfree_rcu() change to kfree() is proper.
>
> Thanks.
Thanks!
Br,
Po Liu
Powered by blists - more mailing lists