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

On Mon, Jun 21, 2021 at 11:32:27AM +0300, Boris Sukholitko wrote:

On Thu, Jun 17, 2021 at 10:51:02PM +0300, Vladimir Oltean wrote:

quoted

On Thu, Jun 17, 2021 at 07:41:55PM +0300, Vladimir Oltean wrote:

quoted

On Thu, Jun 17, 2021 at 07:14:35PM +0300, Vadym Kochan wrote:

[snip excellent problem analysis]

quoted

So maybe it is the flow dissector we need to fix, to make it give us an
additional pure EtherType if asked for, make tc-flower use that
dissector key instead, and then revert Jamal's user space patch, and we
should all install our tc filters as:

tc filter add dev sw1p0 ingress handle 11 protocol all flower eth_type 0x8864 skip_hw action drop

?

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?

Yeah, I don't know about the "_pure_" part (the current name of the
options in tc user space seems fine), but the flow dissector should have
some parsing keys for the C-VLAN and S-VLAN EthType too, since the
FLOW_DISSECTOR_KEY_ETH_TYPE should match on, well, the EtherType.

quoted

Or maybe just be like you, say I don't care about any of that, I just
want it to behave as before, and simply revert Boris's patch. Ok, maybe

FTR I fully support reverting the patch. Please accept my apologies for
breaking the HW offload and big thanks to Vadym for finding it.

I will send the revert shortly.

Thanks,
Boris.

Thanks.

Please note that I haven't used tc for long enough to know what changes
are for its own good, so there is still place for expert feedback from
the maintainers, but this solution seems common sense to me.