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

[Jamal Hadi Salim <jhs@mojatatu.com>]

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?

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?

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?

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.

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

cheers,
jamal