Re: [PATCH net-next] net/sched: cls_flower: Add orig_ethtype

From: Vladimir Oltean <olteanv@gmail.com>
To: Boris Sukholitko <boris.sukholitko@broadcom.com>
Cc: netdev@vger.kernel.org, Jamal Hadi Salim <jhs@mojatatu.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Vadym Kochan <vadym.kochan@plvision.eu>,
	Ilya Lifshits <ilya.lifshits@broadcom.com>
Subject: Re: [PATCH net-next] net/sched: cls_flower: Add orig_ethtype
Date: Mon, 30 Aug 2021 12:00:03 +0300	[thread overview]
Message-ID: <20210830090003.h4hxnb5icwynh7wk@skbuf> (raw)
In-Reply-To: <20210830080800.18591-1-boris.sukholitko@broadcom.com>

Hi Boris,

On Mon, Aug 30, 2021 at 11:08:00AM +0300, Boris Sukholitko wrote:
> The following flower filter fails to match packets:
>
> tc filter add dev eth0 ingress protocol 0x8864 flower \
>     action simple sdata hi64
>
> The protocol 0x8864 (ETH_P_PPP_SES) is a tunnel protocol. As such, it is
> being dissected by __skb_flow_dissect and it's internal protocol is
> being set as key->basic.n_proto. IOW, the existence of ETH_P_PPP_SES
> tunnel is transparent to the callers of __skb_flow_dissect.
>
> OTOH, in the filters above, cls_flower configures its key->basic.n_proto
> to the ETH_P_PPP_SES value configured by the user. Matching on this key
> fails because of __skb_flow_dissect "transparency" mentioned above.
>
> Therefore there is no way currently to match on such packets using
> flower.
>
> To fix the issue add new orig_ethtype key to the flower along with the
> necessary changes to the flow dissector etc.
>
> To filter the ETH_P_PPP_SES packets the command becomes:
>
> tc filter add dev eth0 ingress flower orig_ethtype 0x8864 \
>     action simple sdata hi64
>
> Corresponding iproute2 patch follows.
>
> Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
> ---

It is very good that you've followed up this discussion with a patch:
https://patchwork.kernel.org/project/netdevbpf/patch/20210617161435.8853-1-vadym.kochan@plvision.eu/

I don't seem to see, however, in that discussion, what was the reasoning
that led to the introduction of a new TCA_FLOWER_KEY_ORIG_ETH_TYPE as
opposed to using TCA_FLOWER_KEY_ORIG_ETH_TYPE?

Can you explain in English what is the objective meaning of
TCA_FLOWER_KEY_ORIG_ETH_TYPE, other than "what I need to solve my problem"?
Maybe an entry in the man page section in your iproute2 patch?

How will the VLAN case be dealt with? What is the current status quo on
vlan_ethtype, will a tc-flower key of "vlan_ethtype $((ETH_P_PPP_SES))"
match a VLAN-tagged PPP session packet or not, will the flow dissector
still drill deep inside the packet? I guess this is the reason why you
introduced another variant of the ETH_TYPE netlink attribute, to be
symmetric with what could be done for VLAN? But I don't see VLAN changes?