Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation

Jiri Pirko
2018-11-19T22:46:39Z

Mon, Nov 19, 2018 at 01:15:11AM CET, pablo@netfilter.org wrote:
>This patch implements a new function to translate from native TC action
>to the new flow_action representation. Moreover, this patch also updates
>cls_flower to use this new function.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> include/net/pkt_cls.h | 3 ++
> net/sched/cls_api.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/sched/cls_flower.c | 15 ++++++-
> 3 files changed, 130 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 8b79a1a3a5c7..7d7aefa5fcd2 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -619,6 +619,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> }
> #endif /* CONFIG_NET_CLS_IND */
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts);
>+
> int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> enum tc_setup_type type, void *type_data, bool err_stop);
>
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index d92f44ac4c39..6ab44e650f43 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -31,6 +31,14 @@
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> #include <net/pkt_cls.h>
>+#include <net/tc_act/tc_mirred.h>
>+#include <net/tc_act/tc_vlan.h>
>+#include <net/tc_act/tc_tunnel_key.h>
>+#include <net/tc_act/tc_pedit.h>
>+#include <net/tc_act/tc_csum.h>
>+#include <net/tc_act/tc_gact.h>
>+#include <net/tc_act/tc_skbedit.h>
>+#include <net/tc_act/tc_mirred.h>
>
> extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
>
>@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> }
> EXPORT_SYMBOL(tc_setup_cb_call);
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts)
>+{
>+ const struct tc_action *act;
>+ int num_acts = 0, i, j, k;
>+
>+ if (!exts)
>+ return 0;
>+
>+ tcf_exts_for_each_action(i, act, exts) {
>+ if (is_tcf_pedit(act))
>+ num_acts += tcf_pedit_nkeys(act);
>+ else
>+ num_acts++;
>+ }
>+ if (!num_acts)
>+ return 0;
>+
>+ if (flow_action_init(flow_action, num_acts) < 0)
>+ return -ENOMEM;
>+
>+ j = 0;
>+ tcf_exts_for_each_action(i, act, exts) {
>+ struct flow_action_key *key;
>+
>+ key = &flow_action->keys[j];
>+ if (is_tcf_gact_ok(act)) {
>+ key->id = FLOW_ACTION_KEY_ACCEPT;
>+ } else if (is_tcf_gact_shot(act)) {
>+ key->id = FLOW_ACTION_KEY_DROP;
>+ } else if (is_tcf_gact_trap(act)) {
>+ key->id = FLOW_ACTION_KEY_TRAP;
>+ } else if (is_tcf_gact_goto_chain(act)) {
>+ key->id = FLOW_ACTION_KEY_GOTO;
>+ key->chain_index = tcf_gact_goto_chain_index(act);
>+ } else if (is_tcf_mirred_egress_redirect(act)) {
>+ key->id = FLOW_ACTION_KEY_REDIRECT;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_mirred_egress_mirror(act)) {
>+ key->id = FLOW_ACTION_KEY_MIRRED;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_vlan(act)) {
>+ switch (tcf_vlan_action(act)) {
>+ case TCA_VLAN_ACT_PUSH:
>+ key->id = FLOW_ACTION_KEY_VLAN_PUSH;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ case TCA_VLAN_ACT_POP:
>+ key->id = FLOW_ACTION_KEY_VLAN_POP;
>+ break;
>+ case TCA_VLAN_ACT_MODIFY:
>+ key->id = FLOW_ACTION_KEY_VLAN_MANGLE;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ default:
>+ goto err_out;
>+ }
>+ } else if (is_tcf_tunnel_set(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_ENCAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_tunnel_release(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_DECAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_pedit(act)) {
>+ for (k = 0; k < tcf_pedit_nkeys(act); k++) {
>+ switch (tcf_pedit_cmd(act, k)) {
>+ case TCA_PEDIT_KEY_EX_CMD_SET:
>+ key->id = FLOW_ACTION_KEY_MANGLE;
>+ break;
>+ case TCA_PEDIT_KEY_EX_CMD_ADD:
>+ key->id = FLOW_ACTION_KEY_ADD;
>+ break;
>+ default:
>+ goto err_out;
>+ }
>+ key->mangle.htype = tcf_pedit_htype(act, k);
>+ key->mangle.mask = tcf_pedit_mask(act, k);
>+ key->mangle.val = tcf_pedit_val(act, k);
>+ key->mangle.offset = tcf_pedit_offset(act, k);
>+ key = &flow_action->keys[++j];
>+ }
>+ } else if (is_tcf_csum(act)) {
>+ key->id = FLOW_ACTION_KEY_CSUM;
>+ key->csum_flags = tcf_csum_update_flags(act);
>+ } else if (is_tcf_skbedit_mark(act)) {
>+ key->id = FLOW_ACTION_KEY_MARK;
>+ key->mark = tcf_skbedit_mark(act);
>+ } else {
>+ goto err_out;
>+ }
>+
>+ if (!is_tcf_pedit(act))
>+ j++;
>+ }
>+ return 0;
>+err_out:
>+ flow_action_free(flow_action);
>+ return -EOPNOTSUPP;
>+}
>+EXPORT_SYMBOL(tc_setup_flow_action);
>+
> static __net_init int tcf_net_init(struct net *net)
> {
> struct tcf_net *tn = net_generic(net, tcf_net_id);
>diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>index 26fc129ed504..a301fb8e68e7 100644
>--- a/net/sched/cls_flower.c
>+++ b/net/sched/cls_flower.c
>@@ -104,6 +104,7 @@ struct cls_fl_filter {
> u32 in_hw_count;
> struct rcu_work rwork;
> struct net_device *hw_dev;
>+ struct flow_action action;
> };
>
> static const struct rhashtable_params mask_ht_params = {
>@@ -391,18 +392,27 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
> cls_flower.exts = &f->exts;
> cls_flower.classid = f->res.classid;
>
>+ if (tc_setup_flow_action(&f->action, &f->exts) < 0)
>+ return -ENOMEM;
>+
>+ cls_flower.rule.action.keys = f->action.keys;
>+ cls_flower.rule.action.num_keys = f->action.num_keys;

Hmm, I think flow actions should be only field in rule. Flower does not
use it internally, so it does not really make sense to have f->action

>+
> err = tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
> &cls_flower, skip_sw);
> if (err < 0) {
> fl_hw_destroy_filter(tp, f, NULL);
>+ flow_action_free(&f->action);
> return err;
> } else if (err > 0) {
> f->in_hw_count = err;
> tcf_block_offload_inc(block, &f->flags);
> }
>
>- if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW))
>+ if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW)) {
>+ flow_action_free(&f->action);
> return -EINVAL;
>+ }
>
> return 0;
> }
>@@ -429,6 +439,7 @@ static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> bool async = tcf_exts_get_net(&f->exts);
> bool last;
>
>+ flow_action_free(&f->action);
> idr_remove(&head->handle_idr, f->handle);
> list_del_rcu(&f->list);
> last = fl_mask_put(head, f->mask, async);
>@@ -1470,6 +1481,8 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
> cls_flower.rule.match.mask = &mask->key;
> cls_flower.rule.match.key = &f->mkey;
> cls_flower.exts = &f->exts;
>+ cls_flower.rule.action.num_keys = f->action.num_keys;
>+ cls_flower.rule.action.keys = f->action.keys;
> cls_flower.classid = f->res.classid;
>
> err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
>--
>2.11.0
>