netdev - Re: [PATCH net-next v3] net: openvswitch: Set OvS recirc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1ee4b82c-34d7-7cbb-e445-945f0e52bc31@mellanox.com>
Date:   Wed, 4 Sep 2019 13:01:10 +0000
From:   Paul Blakey <paulb@...lanox.com>
To:     Davide Caratti <dcaratti@...hat.com>,
        Pravin B Shelar <pshelar@....org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Justin Pettit <jpettit@...ira.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Buslov <vladbu@...lanox.com>
CC:     Jiri Pirko <jiri@...lanox.com>, Roi Dayan <roid@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Rony Efraim <ronye@...lanox.com>, Oz Shlomo <ozsh@...lanox.com>
Subject: Re: [PATCH net-next v3] net: openvswitch: Set OvS recirc_id from tc
 chain index


On 9/4/2019 12:47 PM, Davide Caratti wrote:
> On Tue, 2019-09-03 at 16:23 +0300, Paul Blakey wrote:
>> Offloaded OvS datapath rules are translated one to one to tc rules,
>> for example the following simplified OvS rule:
>>
>> recirc_id(0),in_port(dev1),eth_type(0x0800),ct_state(-trk) actions:ct(),recirc(2)
>>
>> Will be translated to the following tc rule:
>>
>> $ tc filter add dev dev1 ingress \
>> 	    prio 1 chain 0 proto ip \
>> 		flower tcp ct_state -trk \
>> 		action ct pipe \
>> 		action goto chain 2
> hello Paul!
>
> one small question:
>
> [... ]
>
>> index 43f5b7e..2fdc746 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -274,7 +274,10 @@ struct tcf_result {
>>   			unsigned long	class;
>>   			u32		classid;
>>   		};
>> -		const struct tcf_proto *goto_tp;
>> +		struct {
>> +			const struct tcf_proto *goto_tp;
>> +			u32 goto_index;
> I don't understand why we need to store another copy of the chain index in
> 'res.goto_index'.
> (see below)
>
> [...]
>
>> index 3397122..c393604 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -27,6 +27,7 @@ static void tcf_action_goto_chain_exec(const struct tc_action *a,
>>   {
>>   	const struct tcf_chain *chain = rcu_dereference_bh(a->goto_chain);
>>   
>> +	res->goto_index = chain->index;
> I see "a->goto_chain" is used to read the chain index, but I think it's
> not needed _ because the chain index is encoded together with the "goto
> chain" control action.
>
>>   	res->goto_tp = rcu_dereference_bh(chain->filter_chain);
>>   }
>>   
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 671ca90..dd147be 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -1514,6 +1514,18 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>   			goto reset;
>>   		} else if (unlikely(TC_ACT_EXT_CMP(err, TC_ACT_GOTO_CHAIN))) {
>>   			first_tp = res->goto_tp;
>> +
>> +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
>> +			{
>> +				struct tc_skb_ext *ext;
>> +
>> +				ext = skb_ext_add(skb, TC_SKB_EXT);
>> +				if (WARN_ON_ONCE(!ext))
>> +					return TC_ACT_SHOT;
>> +
>> +				ext->chain = res->goto_index;
> the value of 'res->goto_index' is already encoded in the control action
> 'err' (masked with TC_ACT_EXT_VAL_MASK), since TC_ACT_GOTO_CHAIN bits are
> not zero.
>
> you can just get rid of res->goto_index, and just do:
>
> 	ext->chain = err & TC_ACT_EXT_VAL_MASK;
>
> am I missing something?
>
> thanks!

No, good catch :) Thanks.

tcf_action_set_ctrlact sets the action with the chain index on tc action 
instance (tcf_action), so yes we can access it just like you say.

I'll send a fix.