netdev - RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM5PR1301MB2172EFE3AC44E84D89D3D081E7609@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date:   Tue, 23 Nov 2021 08:23:44 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Cong Wang <xiyou.wangcong@...il.com>,
        Ido Schimmel <idosch@...dia.com>,
        Jiri Pirko <jiri@...nulli.us>, Oz Shlomo <ozsh@...dia.com>,
        Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers <oss-drivers@...igine.com>
Subject: RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to
 net device

On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:
>On 2021-11-18 08:07, Simon Horman wrote:
>
>[..]
>
> > --- a/net/sched/act_api.c
> > +++ b/net/sched/act_api.c
> > @@ -21,6 +21,19 @@
>> +#include <net/tc_act/tc_pedit.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_csum.h>
>> +#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h>
>> +#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h>
>> +#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h>
>> +#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h>
>>
>>   #ifdef CONFIG_INET
>>   DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);
>> @@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
>>   	kfree(p);
>>   }
>>
>> +static int flow_action_init(struct flow_offload_action *fl_action,
>> +			    struct tc_action *act,
>> +			    enum flow_act_command cmd,
>> +			    struct netlink_ext_ack *extack) {
>> +	if (!fl_action)
>> +		return -EINVAL;
>> +
>> +	fl_action->extack = extack;
>> +	fl_action->command = cmd;
>> +	fl_action->index = act->tcfa_index;
>> +
>> +	if (is_tcf_gact_ok(act)) {
>> +		fl_action->id = FLOW_ACTION_ACCEPT;
>> +	} else if (is_tcf_gact_shot(act)) {
>> +		fl_action->id = FLOW_ACTION_DROP;
>> +	} else if (is_tcf_gact_trap(act)) {
>> +		fl_action->id = FLOW_ACTION_TRAP;
>> +	} else if (is_tcf_gact_goto_chain(act)) {
>> +		fl_action->id = FLOW_ACTION_GOTO;
>> +	} else if (is_tcf_mirred_egress_redirect(act)) {
>> +		fl_action->id = FLOW_ACTION_REDIRECT;
>> +	} else if (is_tcf_mirred_egress_mirror(act)) {
>> +		fl_action->id = FLOW_ACTION_MIRRED;
>> +	} else if (is_tcf_mirred_ingress_redirect(act)) {
>> +		fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
>> +	} else if (is_tcf_mirred_ingress_mirror(act)) {
>> +		fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
>> +	} else if (is_tcf_vlan(act)) {
>> +		switch (tcf_vlan_action(act)) {
>> +		case TCA_VLAN_ACT_PUSH:
>> +			fl_action->id = FLOW_ACTION_VLAN_PUSH;
>> +			break;
>> +		case TCA_VLAN_ACT_POP:
>> +			fl_action->id = FLOW_ACTION_VLAN_POP;
>> +			break;
>> +		case TCA_VLAN_ACT_MODIFY:
>> +			fl_action->id = FLOW_ACTION_VLAN_MANGLE;
>> +			break;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	} else if (is_tcf_tunnel_set(act)) {
>> +		fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
>> +	} else if (is_tcf_tunnel_release(act)) {
>> +		fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
>> +	} else if (is_tcf_csum(act)) {
>> +		fl_action->id = FLOW_ACTION_CSUM;
>> +	} else if (is_tcf_skbedit_mark(act)) {
>> +		fl_action->id = FLOW_ACTION_MARK;
>> +	} else if (is_tcf_sample(act)) {
>> +		fl_action->id = FLOW_ACTION_SAMPLE;
>> +	} else if (is_tcf_police(act)) {
>> +		fl_action->id = FLOW_ACTION_POLICE;
>> +	} else if (is_tcf_ct(act)) {
>> +		fl_action->id = FLOW_ACTION_CT;
>> +	} else if (is_tcf_mpls(act)) {
>> +		switch (tcf_mpls_action(act)) {
>> +		case TCA_MPLS_ACT_PUSH:
>> +			fl_action->id = FLOW_ACTION_MPLS_PUSH;
>> +			break;
>> +		case TCA_MPLS_ACT_POP:
>> +			fl_action->id = FLOW_ACTION_MPLS_POP;
>> +			break;
>> +		case TCA_MPLS_ACT_MODIFY:
>> +			fl_action->id = FLOW_ACTION_MPLS_MANGLE;
>> +			break;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +	} else if (is_tcf_skbedit_ptype(act)) {
>> +		fl_action->id = FLOW_ACTION_PTYPE;
>> +	} else if (is_tcf_skbedit_priority(act)) {
>> +		fl_action->id = FLOW_ACTION_PRIORITY;
>> +	} else if (is_tcf_gate(act)) {
>> +		fl_action->id = FLOW_ACTION_GATE;
>> +	} else {
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>
>The challenge with this is now it is impossible to write an action as a
>standalone module (which works today).
>One resolution to this is to either reuse or introduce a new ops in struct
>tc_action_ops.
>Then flow_action_init() would just invoke this act->ops() which will do action
>specific setup.
>
Thanks for bringing this to us.
As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action. 
If we add a filter with a new added action, we will also fail to offload the filter.
For a new added action, if we aim to offload the action to hardware, then we definitely need a
init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action.

Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this?
>cheers,
>jamal