| 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: Thu, 4 Nov 2021 11:15:24 +0000
From: Baowen Zheng <baowen.zheng@...igine.com>
To: Vlad Buslov <vladbu@...dia.com>
CC: Simon Horman <simon.horman@...igine.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
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 8/8] flow_offload: validate flags of
filter and actions
On November 4, 2021 5:07 PM, Vlad Buslov wrote:
>On Thu 04 Nov 2021 at 07:51, Baowen Zheng <baowen.zheng@...igine.com>
>wrote:
>> Sorry for reply this message again.
>> On November 4, 2021 10:31 AM, Baowen Zheng wrote:
>>>Thanks for your review and sorry for delay in responding.
>>>On October 30, 2021 2:01 AM, Vlad Buslov wrote:
>>>>On Thu 28 Oct 2021 at 14:06, Simon Horman
><simon.horman@...igine.com>
>>>>wrote:
>>>>> From: Baowen Zheng <baowen.zheng@...igine.com>
>>>>>
>>>>> Add process to validate flags of filter and actions when adding a
>>>>> tc filter.
>>>>>
>>>>> We need to prevent adding filter with flags conflicts with its actions.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
>>>>> net/sched/cls_flower.c | 3 ++-
>>>>> net/sched/cls_matchall.c | 4 ++--
>>>>> net/sched/cls_u32.c | 7 ++++---
>>>>> 4 files changed, 34 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
>>>>> 351d93988b8b..80647da9713a 100644
>>>>> --- a/net/sched/cls_api.c
>>>>> +++ b/net/sched/cls_api.c
>>>>> @@ -3025,6 +3025,29 @@ void tcf_exts_destroy(struct tcf_exts *exts)
>>>>> } EXPORT_SYMBOL(tcf_exts_destroy);
>>>>>
>>>>> +static bool tcf_exts_validate_actions(const struct tcf_exts *exts,
>>>>> +u32 flags) { #ifdef CONFIG_NET_CLS_ACT
>>>>> + bool skip_sw = tc_skip_sw(flags);
>>>>> + bool skip_hw = tc_skip_hw(flags);
>>>>> + int i;
>>>>> +
>>>>> + if (!(skip_sw | skip_hw))
>>>>> + return true;
>>>>> +
>>>>> + for (i = 0; i < exts->nr_actions; i++) {
>>>>> + struct tc_action *a = exts->actions[i];
>>>>> +
>>>>> + if ((skip_sw && tc_act_skip_hw(a->tcfa_flags)) ||
>>>>> + (skip_hw && tc_act_skip_sw(a->tcfa_flags)))
>>>>> + return false;
>>>>> + }
>>>>> + return true;
>>>>> +#else
>>>>> + return true;
>>>>> +#endif
>>>>> +}
>>>>> +
>>>>
>>>>I know Jamal suggested to have skip_sw for actions, but it
>>>>complicates the code and I'm still not entirely understand why it is
>necessary.
>>>>After all, action can only get applied to a packet if the packet has
>>>>been matched by some filter and filters already have skip sw/hw
>>>>controls. Forgoing action skip_sw flag would:
>>>>
>>>>- Alleviate the need to validate that filter and action flags are compatible.
>>>>(trying to offload filter that points to existing skip_hw action
>>>>would just fail because the driver wouldn't find the action with
>>>>provided id in its tables)
>>>>
>>>>- Remove the need to add more conditionals into TC software data path
>>>>in patch 4.
>>>>
>>>>WDYT?
>>>As we discussed with Jamal, we will keep the flag of skip_sw and we
>>>need to make exactly match for the actions with flags and the filter
>>>specific action with index.
>>>>
>>>>> int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
>>>>> struct nlattr
>>>**tb,
>>>>> struct nlattr *rate_tlv, struct tcf_exts *exts,
>>>>> u32 flags, struct netlink_ext_ack *extack) @@ -3066,6
>>>>+3089,9
>>>>> @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
>>>>> struct nlattr
>>>>**tb,
>>>>> return err;
>>>>> exts->nr_actions = err;
>>>>> }
>>>>> +
>>>>> + if (!tcf_exts_validate_actions(exts, flags))
>>>>> + return -EINVAL;
>>>>> }
>>>>> #else
>>>>> if ((exts->action && tb[exts->action]) || diff --git
>>>>> a/net/sched/cls_flower.c b/net/sched/cls_flower.c index
>>>>> eb6345a027e1..55f89f0e393e 100644
>>>>> --- a/net/sched/cls_flower.c
>>>>> +++ b/net/sched/cls_flower.c
>>>>> @@ -2035,7 +2035,8 @@ static int fl_change(struct net *net, struct
>>>>> sk_buff
>>>>*in_skb,
>>>>> }
>>>>>
>>>>> err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE],
>>>>> - tp->chain->tmplt_priv, flags, extack);
>>>>> + tp->chain->tmplt_priv, flags | fnew->flags,
>>>>> + extack);
>>>>
>>>>Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_*
>>>>and
>>>>TCA_ACT_FLAGS_*) that map to same bits, or am I missing something?
>>>>This isn't explained in commit message so it is hard for me to
>>>>understand the idea here.
>>>Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to
>>>validate the action flags.
>>>As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16
>>>bits) and the TCA_CLS_FLAGS_* are user flags(in low 16 bits), so they
>>>will not be conflict.
>
>Indeed, currently available TCA_CLS_FLAGS_* fit into first 16 bits, but the field
>itself is 32 bits and with addition of more flags in the future higher bits may
>start to be used since TCA_CLS_FLAGS_* and
>TCA_ACT_FLAGS_* are independent sets.
Thanks, we will use a single parameter as the filter flag.
>
>>>But I think you suggestion also makes sense to us, do you think we
>>>need to pass a single filter flag to make the process more clear?
>> After consideration, I think it is better to separate CLS flags and ACT flags.
>> So we will pass CLS flags as a separate flags, thanks.
>
>Please also validate inside tcf_action_init() instead of creating new
>tcf_exts_validate_actions() function, if possible. I think this will lead to cleaner
>and more simple code.
Thanks, we will consider to implement the validation inside tcf_action_init().
>
>>>>
>>>>> if (err)
>>>>> goto errout;
>>>>>
>>>>> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
>>>>> index 24f0046ce0b3..00b76fbc1dce 100644
>>>>> --- a/net/sched/cls_matchall.c
>>>>> +++ b/net/sched/cls_matchall.c
>>>>> @@ -226,8 +226,8 @@ static int mall_change(struct net *net, struct
>>>>> sk_buff
>>>>*in_skb,
>>>>> goto err_alloc_percpu;
>>>>> }
>>>>>
>>>>> - err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE], flags,
>>>>> - extack);
>>>>> + err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE],
>>>>> + flags | new->flags, extack);
>>>>> if (err)
>>>>> goto err_set_parms;
>>>>>
>>>>> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index
>>>>> 4272814487f0..fc670cc45122 100644
>>>>> --- a/net/sched/cls_u32.c
>>>>> +++ b/net/sched/cls_u32.c
>>>>> @@ -895,7 +895,8 @@ static int u32_change(struct net *net, struct
>>>>> sk_buff
>>>>*in_skb,
>>>>> return -ENOMEM;
>>>>>
>>>>> err = u32_set_parms(net, tp, base, new, tb,
>>>>> - tca[TCA_RATE], flags, extack);
>>>>> + tca[TCA_RATE], flags | new->flags,
>>>>> + extack);
>>>>>
>>>>> if (err) {
>>>>> u32_destroy_key(new, false);
>>>>> @@ -1060,8 +1061,8 @@ static int u32_change(struct net *net, struct
>>>>sk_buff *in_skb,
>>>>> }
>>>>> #endif
>>>>>
>>>>> - err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], flags,
>>>>> - extack);
>>>>> + err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE],
>>>>> + flags | n->flags, extack);
>>>>> if (err == 0) {
>>>>> struct tc_u_knode __rcu **ins;
>>>>> struct tc_u_knode *pins;
Powered by blists - more mailing lists