| 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
| ||
|
Date: Thu, 22 Jul 2021 17:25:13 +0300
From: Vlad Buslov <vladbu@...dia.com>
To: Simon Horman <simon.horman@...igine.com>
CC: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...lanox.com>, <netdev@...r.kernel.org>,
<oss-drivers@...igine.com>,
Baowen Zheng <baowen.zheng@...igine.com>,
"Louis Peens" <louis.peens@...igine.com>
Subject: Re: [PATCH net-next 2/3] flow_offload: add process to delete
offloaded actions from net device
On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@...igine.com> wrote:
> From: Baowen Zheng <baowen.zheng@...igine.com>
>
> Add a basic process to delete offloaded actions from net device.
>
> Should not remove the offloaded action entries if the action
> fails to delete in tcf_del_notify.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@...igine.com>
> Signed-off-by: Louis Peens <louis.peens@...igine.com>
> Signed-off-by: Simon Horman <simon.horman@...igine.com>
> ---
> include/net/pkt_cls.h | 1 +
> net/sched/act_api.c | 112 +++++++++++++++++++++++++++++++++++-------
> net/sched/cls_api.c | 14 ++++--
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index cd4cf6b10f5d..03dae225d64f 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -573,6 +573,7 @@ int tc_setup_cb_reoffload(struct tcf_block *block, struct tcf_proto *tp,
> void *cb_priv, u32 *flags, unsigned int *in_hw_count);
> unsigned int tcf_exts_num_actions(struct tcf_exts *exts);
> unsigned int tcf_act_num_actions(struct tc_action *actions[]);
> +unsigned int tcf_act_num_actions_single(struct tc_action *act);
>
> #ifdef CONFIG_NET_CLS_ACT
> int tcf_qevent_init(struct tcf_qevent *qe, struct Qdisc *sch,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 185f17ea60d5..23a4538916af 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1060,36 +1060,109 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> return ERR_PTR(err);
> }
>
> -/* offload the tc command after inserted */
> -int tcf_action_offload_cmd(struct tc_action *actions[],
> - struct netlink_ext_ack *extack)
> +int tcf_action_offload_cmd_pre(struct tc_action *actions[],
> + enum flow_act_command cmd,
> + struct netlink_ext_ack *extack,
> + struct flow_offload_action **fl_act)
> {
> - struct flow_offload_action *fl_act;
> + struct flow_offload_action *fl_act_p;
> int err = 0;
>
> - fl_act = flow_action_alloc(tcf_act_num_actions(actions));
> - if (!fl_act)
> + fl_act_p = flow_action_alloc(tcf_act_num_actions(actions));
> + if (!fl_act_p)
> return -ENOMEM;
>
> - fl_act->extack = extack;
> - err = tc_setup_action(&fl_act->action, actions);
> + fl_act_p->extack = extack;
> + fl_act_p->command = cmd;
> + err = tc_setup_action(&fl_act_p->action, actions);
> if (err) {
> NL_SET_ERR_MSG_MOD(extack,
> "Failed to setup tc actions for offload\n");
> goto err_out;
> }
> - fl_act->command = FLOW_ACT_REPLACE;
> + *fl_act = fl_act_p;
> + return 0;
> +err_out:
> + kfree(fl_act_p);
> + return err;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
This doesn't seem be used anywhere outside this file.
> +
> +int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
> + struct netlink_ext_ack *extack)
> +{
> + if (IS_ERR(fl_act))
> + return PTR_ERR(fl_act);
>
> flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
>
> tc_cleanup_flow_action(&fl_act->action);
> -
> -err_out:
> kfree(fl_act);
> - return err;
> + return 0;
> +}
This one is not exported, by is non-static.
> +
> +/* offload the tc command after inserted */
> +int tcf_action_offload_cmd(struct tc_action *actions[],
> + struct netlink_ext_ack *extack)
> +{
> + struct flow_offload_action *fl_act;
> + int err = 0;
> +
> + err = tcf_action_offload_cmd_pre(actions,
> + FLOW_ACT_REPLACE,
> + extack,
> + &fl_act);
> + if (err)
> + return err;
> +
> + return tcf_action_offload_cmd_post(fl_act, extack);
> }
> EXPORT_SYMBOL(tcf_action_offload_cmd);
>
> +/* offload the tc command after deleted */
> +int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
> + struct tc_action *actions[],
> + struct netlink_ext_ack *extack,
> + int fallback_num)
> +{
> + int fallback_entries = 0;
> + struct tc_action *act;
> + int total_entries = 0;
> + int i;
> +
> + if (!fl_act)
> + return -EINVAL;
> +
> + if (fallback_num) {
> + /* for each the actions to fallback the action entries remain in the actions */
> + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
> + act = actions[i];
> + if (!act)
> + continue;
> +
> + fallback_entries += tcf_act_num_actions_single(act);
> + }
> + fallback_entries += fallback_num;
> + }
> + total_entries = fl_act->action.num_entries;
> + if (total_entries > fallback_entries) {
> + /* just offload the actions that is not fallback and start with the actions */
> + fl_act->action.num_entries -= fallback_entries;
> + flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
> +
> + /* recovery num_entries for cleanup */
> + fl_act->action.num_entries = total_entries;
> + } else {
> + NL_SET_ERR_MSG(extack, "no entries to offload when deleting the tc actions");
> + }
> +
> + tc_cleanup_flow_action(&fl_act->action);
> +
> + kfree(fl_act);
> + return 0;
> +}
> +EXPORT_SYMBOL(tcf_action_offload_del_post);
> +
> /* Returns numbers of initialized actions or negative error. */
>
> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> @@ -1393,7 +1466,7 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
> return err;
> }
>
> -static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> +static int tcf_action_delete(struct net *net, struct tc_action *actions[], int *fallbacknum)
> {
> int i;
>
> @@ -1407,6 +1480,7 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> u32 act_index = a->tcfa_index;
>
> actions[i] = NULL;
> + *fallbacknum = tcf_act_num_actions_single(a);
> if (tcf_action_put(a)) {
> /* last reference, action was deleted concurrently */
> module_put(ops->owner);
> @@ -1419,12 +1493,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
> return ret;
> }
> }
> + *fallbacknum = 0;
> return 0;
> }
>
> static int
> tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
> - u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
> + u32 portid, size_t attr_size, struct netlink_ext_ack *extack, int *fallbacknum)
> {
> int ret;
> struct sk_buff *skb;
> @@ -1442,7 +1517,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
> }
>
> /* now do the delete */
> - ret = tcf_action_delete(net, actions);
> + ret = tcf_action_delete(net, actions, fallbacknum);
> if (ret < 0) {
> NL_SET_ERR_MSG(extack, "Failed to delete TC action");
> kfree_skb(skb);
> @@ -1458,11 +1533,12 @@ static int
> tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> u32 portid, int event, struct netlink_ext_ack *extack)
> {
> - int i, ret;
> struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
> struct tc_action *act;
> size_t attr_size = 0;
> struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
> + struct flow_offload_action *fl_act;
> + int i, ret, fallback_num;
>
> ret = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
> extack);
> @@ -1492,7 +1568,9 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
> if (event == RTM_GETACTION)
> ret = tcf_get_notify(net, portid, n, actions, event, extack);
> else { /* delete */
> - ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
> + tcf_action_offload_cmd_pre(actions, FLOW_ACT_DESTROY, extack, &fl_act);
> + ret = tcf_del_notify(net, n, actions, portid, attr_size, extack, &fallback_num);
> + tcf_action_offload_del_post(fl_act, actions, extack, fallback_num);
This tcf_action_offload_cmd_{pre|post}() approach looks slightly
complicated, especially with fallback_num calculations. I would suggest
to simplify it by only initializing action cookies in
flow_action->entries[] (I assume you don't really need all the action
data just to delete it, right?) for DEL/STATS and do one of the
following:
- Unoffload actions one-by-one after every deletion in
tcf_actions_delete(), perhaps reusing the same flow_offload_action of
size 1 by only changing the cookie on each iteration.
- If you really want to send the whole batch to the driver, save cookies
for all successfully deleted actions in an array and initialize
compound flow_offload_action from the array.
This would remove the need for whole pre/post thing, which otherwise
gets even more complicated in following patch by 'keep_fl_act' arg.
> if (ret)
> goto err;
> return 0;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 9b9770dab5e8..23ce021f07f8 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3755,6 +3755,15 @@ unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> }
> EXPORT_SYMBOL(tcf_exts_num_actions);
>
> +unsigned int tcf_act_num_actions_single(struct tc_action *act)
> +{
> + if (is_tcf_pedit(act))
> + return tcf_pedit_nkeys(act);
> + else
> + return 1;
> +}
> +EXPORT_SYMBOL(tcf_act_num_actions_single);
> +
> unsigned int tcf_act_num_actions(struct tc_action *actions[])
> {
> unsigned int num_acts = 0;
> @@ -3762,10 +3771,7 @@ unsigned int tcf_act_num_actions(struct tc_action *actions[])
> int i;
>
> tcf_act_for_each_action(i, act, actions) {
> - if (is_tcf_pedit(act))
> - num_acts += tcf_pedit_nkeys(act);
> - else
> - num_acts++;
> + num_acts += tcf_act_num_actions_single(act);
> }
> return num_acts;
> }
Powered by blists - more mailing lists