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: 4699 bytes --]

On Tue, Jun 22, 2021 at 10:17:45AM -0400, Jamal Hadi Salim wrote:
> Hi Boris,
> 
> On 2021-06-22 9:13 a.m., Boris Sukholitko wrote:
> > Hi Jamal,
> > 
> > On Mon, Jun 21, 2021 at 10:04:41AM -0400, Jamal Hadi Salim wrote:
> > > On 2021-06-21 4:32 a.m., Boris Sukholitko wrote:
> 
> [..]
> > > > I like this solution. To be more explicit, the plan becomes:
> > > > 
> > > > 1. Add FLOW_DISSECTOR_KEY_ETH_TYPE and struct flow_dissector_key_eth_type.
> > > > 2. Have skb flow dissector use it.
> > > > 3. Userspace does not set TCA_FLOWER_KEY_ETH_TYPE automagically
> > > >      anymore. cls_flower takes basic.n_proto from struct tcf_proto.
> > > > 4. Add eth_type to the userspace and use it to set TCA_FLOWER_KEY_ETH_TYPE
> > > > 5. Existence of TCA_FLOWER_KEY_ETH_TYPE triggers new eth_type dissector.
> > > > 
> > > > IMHO this neatly solves non-vlan protocol match case.
> > > > 
> > > > What should we do with the VLANs then? Should we have vlan_pure_ethtype
> > > > and cvlan_pure_ethtype as additional keys?
> > > > 
> > > 
> > > I didnt see the original patch you sent until after it was applied
> > > and the cursory 30 second glance didnt say much to me.
> > > 
> > > Vlans unfortunately are a speacial beast: You will have to retrieve
> > > the proto differently.
> > 
> > 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.

> 
> Before we go there let me push back a little since no other
> classifier has this problem. IIUC:
> For the hardware offload current scheme works fine. On the
> non-offload side, the issue seems to be that classify() was
> not getting the proper protocol?

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.

> 
> I pointed to Toke's change since it tries to generalize the
> solution.  you'd use that approach
> (eg setting to true).
> 
> Would that solve the issue?
> 

I don't see how it might help ...

> If not maybe we need a naming convention for inner vs out proto.
> Should be noted that user space semantics require that the "current
> protocol" be specified in the policy. i.e if you have an inner
> protocol and you need both looked at then you would have two rules
> like this:
> 
> 1) tc filter ... protocol outer .... action-to-strip-outer-header \
> action reclassify
> 2) tc filter ... protoco inner ... action blah
> 
> The action-to-strip-outer-header will set properly the skb->proto
> after moving the data pointers so that #2 will match it.

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.

> 
> > > 
> > > Q: Was this always broken? Example look at Toke's change here:
> > > commit d7bf2ebebc2bd61ab95e2a8e33541ef282f303d4
> > > 
> > 
> > IMHO we've always had this problem. I did some archeology on this
> > code and didn't see anything which might have caused the bug.
> > 
> 
> Suprised it took this long to find out.
> 
> > Toke's change doesn't look related because in fl_classify it does:
> > 
> > 	skb_key.basic.n_proto = skb_protocol(skb, false);
> > 
> > before running the dissector. In case of a known tunnelling protocol (such
> > as ETH_P_PPP_SES) the n_proto will be overriden by __skb_flow_dissect.
> > 
> 
> Toke's change may have left things as they were before
> but generally to get vlan protos you'd do things differently.
> 
> 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.

Thanks,
Boris.

> 
> cheers,
> jamal
> 

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