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

[Jamal Hadi Salim]

Hi Boris,

Apologies for the latency.

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. 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).


> 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..

> > 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?

cheers,
jamal