| 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: <DM5PR1301MB217274AAA2D474D194159C8BE78A9@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date: Mon, 1 Nov 2021 10:27:33 +0000
From: Baowen Zheng <baowen.zheng@...igine.com>
To: Oz Shlomo <ozsh@...dia.com>,
Simon Horman <simon.horman@...igine.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: Vlad Buslov <vladbu@...dia.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Roi Dayan <roid@...dia.com>, Ido Schimmel <idosch@...dia.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>,
Baowen Zheng <notifications@...hub.com>,
Louis Peens <louis.peens@...igine.com>,
oss-drivers <oss-drivers@...igine.com>
Subject: RE: [RFC/PATCH net-next v3 3/8] flow_offload: allow user to offload
tc action to net device
On November 1, 2021 6:07 PM, Oz Shlomo wrote:
>On 11/1/2021 4:30 AM, Baowen Zheng wrote:
>> On 10/31/2021 5:50 PM, Oz Shlomo wrote:
>>> On 10/28/2021 2:06 PM, Simon Horman wrote:
>>>> From: Baowen Zheng <baowen.zheng@...igine.com>
>>>>
>>>> Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
>>>> action.
>>>
>>> How will device drivers reference the offloaded actions when
>>> offloading a flow?
>>> Perhaps the flow_action_entry structure should also include the action
>index.
>>>
>> We have set action index in flow_offload_action to offload the action, also
>there are > already some actions in flow_action_entry include index which we
>want to offload.
>> If the driver wants to support action that needs index, I think it can
>> add the index later, it may not include in this patch, WDYT?
>
>What do you mean by "action that needs index"?
>
>Currently only the police and gate actions have an action index parameter.
>However, with this series the user can create any action using the tc action API
>and then reference it from any filter.
>Do you see a reason not to expose the action index as a flow_action_entry
>attribute?
What I mean is currently the action is created along with the filter, then the index is not needed.
With this patch, we intend to offload the police action which already includes action index.
I think your suggestion makes sense to us, we will consider to move the index to the
flow_action_entry structure instead of current in single action structure, thanks.
>>>>
>>>> We need to call tc_cleanup_flow_action to clean up tc action entry
>>>> since in tc_setup_action, some actions may hold dev refcnt, especially
>>>> the mirror action.
>>>>
>>>> Signed-off-by: Baowen Zheng <baowen.zheng@...igine.com>
>>>> Signed-off-by: Louis Peens <louis.peens@...igine.com>
>>>> Signed-off-by: Simon Horman <simon.horman@...igine.com>
>>>> ---
>>>> include/linux/netdevice.h | 1 +
>>>> include/net/act_api.h | 2 +-
>>>> include/net/flow_offload.h | 17 ++++
>>>> include/net/pkt_cls.h | 15 ++++
>>>> net/core/flow_offload.c | 43 ++++++++--
>>>> net/sched/act_api.c | 166
>+++++++++++++++++++++++++++++++++++++
>>>> net/sched/cls_api.c | 29 ++++++-
>>>> 7 files changed, 260 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 3ec42495a43a..9815c3a058e9 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -916,6 +916,7 @@ enum tc_setup_type {
>>>> TC_SETUP_QDISC_TBF,
>>>> TC_SETUP_QDISC_FIFO,
>>>> TC_SETUP_QDISC_HTB,
>>>> + TC_SETUP_ACT,
>>>> };
>>>>
...
>>>> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
>>>> nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net,
>>> struct tcf_proto *tp, struct nlattr *nla,
>>>> sz += tcf_action_fill_size(act);
>>>> /* Start from index 0 */
>>>> actions[i - 1] = act;
>>>> + if (!(flags & TCA_ACT_FLAGS_BIND))
>>>> + tcf_action_offload_add(act, extack);
>>>
>>> Why is this restricted to actions created without the
>TCA_ACT_FLAGS_BIND
>>> flag?
>>> How are actions instantiated by the filters different from those that are
>>> created by "tc actions"?
>>>
>> Our patch aims to offload tc action that is created independent of any flow.
>It is usually
>> offloaded when it is added or replaced.
>> This patch is to implement a process of reoffloading the actions when driver
>is
>> inserted or removed, so it will still offload the independent actions.
>
>I see.
>
>>>> }
>>>>
>>>> /* We have to commit them all together, because if any error
>>>> happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>>> index 2ef8f5a6205a..351d93988b8b 100644
>>>> --- a/net/sched/cls_api.c
>>>> +++ b/net/sched/cls_api.c
>>>> @@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats
>>> tc_act_hw_stats(u8 hw_stats)
>>>> return hw_stats;
>>>> }
>>>>
>>>> -int tc_setup_flow_action(struct flow_action *flow_action,
>>>> - const struct tcf_exts *exts)
>>>> +int tc_setup_action(struct flow_action *flow_action,
>>>> + struct tc_action *actions[])
>>>> {
>>>> struct tc_action *act;
>>>> int i, j, k, err = 0;
>>>> @@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action
>>> *flow_action,
>>>> BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE !=
>>> FLOW_ACTION_HW_STATS_IMMEDIATE);
>>>> BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED !=
>>>> FLOW_ACTION_HW_STATS_DELAYED);
>>>>
>>>> - if (!exts)
>>>> + if (!actions)
>>>> return 0;
>>>>
>>>> j = 0;
>>>> - tcf_exts_for_each_action(i, act, exts) {
>>>> + tcf_act_for_each_action(i, act, actions) {
>>>> struct flow_action_entry *entry;
>>>>
>>>> entry = &flow_action->entries[j];
>>>> @@ -3725,7 +3725,19 @@ int tc_setup_flow_action(struct flow_action
>>> *flow_action,
>>>> spin_unlock_bh(&act->tcfa_lock);
>>>> goto err_out;
>>>> }
>>>> +EXPORT_SYMBOL(tc_setup_action);
>>>> +
>>>> +#ifdef CONFIG_NET_CLS_ACT
>>>> +int tc_setup_flow_action(struct flow_action *flow_action,
>>>> + const struct tcf_exts *exts)
>>>> +{
>>>> + if (!exts)
>>>> + return 0;
>>>> +
>>>> + return tc_setup_action(flow_action, exts->actions); }
>>>> EXPORT_SYMBOL(tc_setup_flow_action);
>>>> +#endif
>>>>
>>>> unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
>>>> {
>>>> @@ -3743,6 +3755,15 @@ unsigned int tcf_exts_num_actions(struct
>>> tcf_exts *exts)
>>>> }
>>>> EXPORT_SYMBOL(tcf_exts_num_actions);
>>>>
>>>> +unsigned int tcf_act_num_actions_single(struct tc_action *act) {
>>>> + if (is_tcf_pedit(act))
>>>> + return tcf_pedit_nkeys(act);
>>>> + else
>>>> + return 1;
>>>> +}
>>>> +EXPORT_SYMBOL(tcf_act_num_actions_single);
>>>> +
>>>> #ifdef CONFIG_NET_CLS_ACT
>>>> static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
>>>> u32 *p_block_index,
>>>>
Powered by blists - more mailing lists