| 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: <DM5PR1301MB2172ED85399FCC4B89F70792E7619@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date: Wed, 24 Nov 2021 02:59:08 +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
Sorry for reply this message again.
On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>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.
Maybe I misunderstand what you mean previously, when I look through the implement in
flow_action_init, I did not found we use the extack to make a log before return -EINVAL.
So could you please figure it out? Maybe I miss something or misunderstand again.
>>cheers,
>>jamal
Powered by blists - more mailing lists