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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b56001da1c3795ff9bb18a2aded62dea360faf9.camel@redhat.com>
Date:   Wed, 04 Sep 2019 11:47:11 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Paul Blakey <paulb@...lanox.com>,
        Pravin B Shelar <pshelar@....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 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!
-- 
davide