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

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: <20210831120440.GA4641@noodle>
Date:   Tue, 31 Aug 2021 15:04:40 +0300
From:   Boris Sukholitko <boris.sukholitko@...adcom.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <olteanv@...il.com>,
        Vadym Kochan <vadym.kochan@...ision.eu>,
        Ilya Lifshits <ilya.lifshits@...adcom.com>,
        tom Herbert <tom@...anda.io>,
        Felipe Magno de Almeida <felipe@...ertise.dev>,
        Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next] net/sched: cls_flower: Add orig_ethtype

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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)