Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Boris Sukholitko <boris.sukholitko@broadcom.com>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: netdev@vger.kernel.org, Jiri Pirko <jiri@resnulli.us>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Vladimir Oltean <olteanv@gmail.com>,
	Vadym Kochan <vadym.kochan@plvision.eu>,
	Ilya Lifshits <ilya.lifshits@broadcom.com>,
	tom Herbert <tom@sipanda.io>,
	Felipe Magno de Almeida <felipe@expertise.dev>,
	Pedro Tammela <pctammela@mojatatu.com>
Subject: Re: [PATCH net-next] net/sched: cls_flower: Add orig_ethtype
Date: Tue, 31 Aug 2021 15:04:40 +0300	[thread overview]
Message-ID: <20210831120440.GA4641@noodle> (raw)
In-Reply-To: <b05f2736-fa76-4071-3d52-92ac765ca405@mojatatu.com>

[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]

Hi Jamal,

On Mon, Aug 30, 2021 at 09:48:38PM -0400, Jamal Hadi Salim wrote:
> On 2021-08-30 4:08 a.m., 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
> 
> Where's "protocol" on the above command line is. Probably a typo?

There is no need for protocol there. We intend to match on the tunnel
protocol existence only, disregarding its contents. Therefore
orig_ethtype key is sufficient.

> 
> The main culprit is clearly the flow dissector parsing. I am not sure
> if in general flowdisc to deal with deeper hierarchies/tunnels
> without constantly adding a lot more hacks. Imagine if you had an
> ethernet packet with double vlan tags and encapsulating a pppoe packet
> (i.e 3 or more layers of ethernet) - that would be a nightmare.

Of course there is no limit to our imagination :) However I would argue
that in the RealWorld(tm) the number of such nesting cases is
neglectable.

The evidence is that TC and flower are being actively used. Double VLAN
tags configurations notwithstading. IMHO, the fact that I've been
unlucky to hit this tunnel corner case does not mean that there is a
design problem with the flower.

AFAICS, the current meaning for the protocol field in TC is:

match the innermost layer 2 type through the tunnels that the kernel
knows about.

And it seems to me that this semantic is perfectly fine and does not
require fixing. Maybe be we need to mention it in the docs...

> IMO, your approach is adding yet another bandaid.

Could you please elaborate why do you see this approach as a bandaid? 

The patch in question adds another key to the other ~50 that exists in the
flower currently. Two more similar keys will be done for single and
double VLAN case. As a result, my users will gain the ability to match
packets that are impossible to match right now.

In difference with the TC protocol field, orig_ethtype answers the
question:

what is the original eth type of the packet, independent of the further
kernel processing.

IMHO, the above definition is also quite exact and has the right to
exist because we do not have such ability in the current kernel.

> 
> Would it make sense for the setting of the
> skb_key.basic.n_proto  to be from tp->protocol for
> your specific case in classify().
> 
> Which means your original setup:
>  tc filter add dev eth0 ingress protocol 0x8864 flower \
>      action simple sdata hi64
> 
> should continue to work if i am not mistaken. Vlans would
> continue to be a speacial case.
> 
> I dont know what that would break though...
> 

I think right off the bat, it will break the following user
configuration (untested!):

tc filter add dev eth0 ingress protocol ipv4 flower \
      action simple sdata hi64

Currently, the above rule matches ipv4 packets encapsulated in
ETH_P_PPP_SES protocol. The special casing will break it.

Thanks,
Boris.

> cheers,
> jamal

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

  reply	other threads:[~2021-08-31 12:05 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-30  8:08 Boris Sukholitko
2021-08-30  9:00 ` Vladimir Oltean
2021-08-30  9:04   ` Vladimir Oltean
2021-08-30  9:18   ` Boris Sukholitko
2021-08-30  9:21     ` Vladimir Oltean
2021-08-30  9:42       ` Boris Sukholitko
2021-08-30 10:13         ` Vladimir Oltean
2021-08-31  1:48 ` Jamal Hadi Salim
2021-08-31 12:04   ` Boris Sukholitko [this message]
2021-08-31 13:18     ` Jamal Hadi Salim
2021-08-31 14:03       ` Boris Sukholitko
2021-09-02  6:48       ` Ido Schimmel
2021-09-03 22:52         ` Jamal Hadi Salim
2021-09-04 14:08       ` Tom Herbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210831120440.GA4641@noodle \
    --to=boris.sukholitko@broadcom.com \
    --cc=davem@davemloft.net \
    --cc=felipe@expertise.dev \
    --cc=ilya.lifshits@broadcom.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=pctammela@mojatatu.com \
    --cc=tom@sipanda.io \
    --cc=vadym.kochan@plvision.eu \
    --cc=xiyou.wangcong@gmail.com \
    --subject='Re: [PATCH net-next] net/sched: cls_flower: Add orig_ethtype' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).