Re: [PATCH net-next] net/sched: cls_flower: fix resetting of ether proto mask

[Boris Sukholitko <boris.sukholitko@broadcom.com>]

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

Hi Jamal,

On Thu, Jun 24, 2021 at 04:07:56PM -0400, Jamal Hadi Salim wrote:
> Hi Boris,
> 
> Apologies for the latency.
> 

Apparently mine is not great either. :)

> On 2021-06-22 11:22 a.m., Boris Sukholitko wrote:
> > On Tue, Jun 22, 2021 at 10:17:45AM -0400, Jamal Hadi Salim wrote:
> 
> [..]
> 
> > > > Do you by any chance have some naming suggestion? Does
> > > > vlan_pure_ethtype sound ok? What about vlan_{orig, pkt, raw, hdr}_ethtype?
> > > > 
> > > 
> > > The distinction is in getting the inner vs outer proto, correct?
> > 
> > Yes. To be more explicit: the outer protocol (ETH_P_PPP_SES in this case) is
> > invisible to the user due to __skb_flow_dissect drilling down
> > to find the inner protocol.
> 
> Ok, seems this is going to be problematic for flower for more than
> just ETH_P_PPP_SES, no? i.e anything that has an inner proto.
> IIUC, basically what you end up seeing in fl_classify() is
> the PPP protocol that is extracted by the dissector?
> 

Yes. It looks like the problem for all of tunnel protocols.

> > Yes. Talking specifically about flower's fl_classify and the following
> > rule (0x8864 is ETH_P_PPP_SES):
> > 
> > tc filter add dev eth0 ingress protocol 0x8864 flower action simple sdata hi6
> > 
> > skb_flow_dissect sets skb_key.basic.n_proto to the inner protocol
> > contained inside the PPP tunnel. fl_mask_lookup will fail finding the
> > outer protocol configured by the user.
> > 
> 
> For vlans it seems that flower tries to "rectify" the situation
> with skb_protocol() (that why i pointed to that function) - but the
> situation in this case cant be rectified inside fl_classify().
> 
> Just quick glance of the dissector code though seems to indicate
> skb->protocol is untouched, no? i.e if you have a simple pppoe with
> ppp protocol == ipv4, skb->protocol should still be 0x8864 on it
> (even when skb_key.basic.n_proto has ipv4).
> 

The skb->protocol is probably untouched. Unfortunately I don't see
how this may help us... 

> 
> > It looks to me that there is no way to match on outer protocol such as
> > ETH_P_PPP_SES at least in flower. Although other filters (e.g. matchall)
> > will work, VLAN packets containing ETH_P_PPP_SES will require flower and
> > still will not match.
> 
> This is a consequence of flower using flow_dissector and flow
> dissector loosing information..
> 

Yes.

> > > This is because when vlan offloading was merged it skewed
> > > things a little and we had to live with that.
> > > 
> > > Flower is unique in its use of the dissector which other classifiers
> > > dont. Is this resolvable by having the fix in the  dissector?
> > 
> > Yes, the solution suggested by Vladimir and elaborated by myself
> > involves extending the dissector to keep the outer protocol and having
> > flower eth_type match on it. This is the "plan" being quoted above.
> > 
> >
> > I believe this is the solution for the non-vlan tagged traffic. For the
> > vlans we already have [c]vlan_ethtype keys taken. Therefore we'll need
> > new [c]vlan_outer_ethtype keys.
> > 
> 
> I think that would work in the short term but what happens when you
> have vlan in vlan carrying pppoe? i.e how much max nesting can you
> assume and what would you call these fields?
> 

Currently flower matches on 2 level of vlans: e.g. it has vlan_prio for
the outer vlan and cvlan_prio for the inner vlan. I have no ambition to
go beyond that. Thus only two keys will be needed: vlan_outer_ethtype
and cvlan_outer_ethtype, I think...

In your vlan in vlan ppoe example, I hope that
cvlan_outer_ethtype == ETH_P_PPP_SES will match.

Thanks,
Boris.

> cheers,
> jamal
> 

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