| 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: Wed, 24 Nov 2021 02:11:00 +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 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>On 2021-11-23 03:23, Baowen Zheng wrote:
>> 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.
>>
>
>The simplest approach seems to be adding a field in ops struct and call it
>hw_id (we already have id which represents the s/w side).
>All your code in flow_action_init() then becomes something like:
>
> if (!fl_action)
> return -EINVAL;
>
> fl_action->extack = extack;
> fl_action->command = cmd;
> fl_action->index = act->tcfa_index;
>
>
> fl_action->id = act->hwid;
>
>And modules continue to work. Did i miss something?
>
Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions
that will be assigned different hw_id in different case, such as the gact, we need to think about this case.
So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added
Standalone action.
WDYT?
>> Do you think it is proper to include this implement in our patch series or we
>can delivery a new patch for this?
>
>Unless I am missing something basic, I dont see this as hard to do as explained
>above in this patch series.
I did not mean it is difficult.
Since as my understanding, we will have the same problem in function of tc_setup_flow_action to
Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement
Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish.
What is your opinion?
>
>BTW: shouldnt extack be used here instead of returning just -EINVAL?
>I didnt stare long enough but it seems extack is not passed when deleting from
>hardware? I saw a NULL being passed in one of the patches.
Ok we will consider to delete the extack parameter.
>cheers,
>jamal
Powered by blists - more mailing lists