| 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: <404a4871-0e12-3cdc-e8c7-b0c85e068c53@mojatatu.com>
Date: Tue, 23 Nov 2021 14:03:46 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Baowen Zheng <baowen.zheng@...igine.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 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?
> 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.
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.
cheers,
jamal
Powered by blists - more mailing lists