Re: [net-next,v4] net: dsa: mv88e6xxx: Add partial support for TCAM entries

[Cedric Jehasse]

>> >> +     if (dissector->used_keys &
>> >> +         ~(BIT_ULL(FLOW_DISSECTOR_KEY_BASIC) |
>> >> +           BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL) |
>> >> +           BIT_ULL(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
>> >> +           BIT_ULL(FLOW_DISSECTOR_KEY_IPV6_ADDRS))) {
>> >> +             NL_SET_ERR_MSG_MOD(extack,
>> >> +                                "Unsupported keys used");
>> >> +             return -EOPNOTSUPP;
>> >> +     }
>> >
>> >FLOW_DISSECTOR_KEY_IPV6_ADDRS is included in the allowed keys bitmask,
>> >but there is no code below to parse IPv6 addresses into the TCAM key.
>> >
>> >If a user creates a tc flower rule with IPv6 addresses, the rule passes
>> >this validation check but the addresses are never programmed into the
>> >TCAM. This would cause the hardware entry to match broader traffic than
>> >intended.
>> >
>> >Looking at the code further down:
>> >
>> >> +     if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
>> >> +             struct flow_match_ipv4_addrs match;
>> >> +
>> >> +             flow_rule_match_ipv4_addrs(cls->rule, &match);
>> >> +             mv88e6xxx_tcam_match_set(key, MV88E6XXX_IPV4_SRC_OFFSET,
>> >> +                                      match.key->src,
>> >> +                                      match.mask->src);
>> >> +             mv88e6xxx_tcam_match_set(key, MV88E6XXX_IPV4_DST_OFFSET,
>> >> +                                      match.key->dst,
>> >> +                                      match.mask->dst);
>> >> +     } else {
>> >> +             NL_SET_ERR_MSG_MOD(extack,
>> >> +                                "Unsupported address type");
>> >> +             return -EOPNOTSUPP;
>> >> +     }
>> >
>> >The check at line 68-72 rejects non-IPv4 address types, but the IPv6 key
>> >is still in the allowed keys bitmask at line 30. This creates a confusing
>> >situation where IPv6 is both 'allowed' and 'unsupported'.
>> >
>> >Should FLOW_DISSECTOR_KEY_IPV6_ADDRS be removed from the allowed keys
>> >until IPv6 parsing is implemented?
>>
>> At first FLOW_DISSECTOR_KEY_IPV6_ADDRS wasn't in the allowed keys bitmask, but
>> creating ipv4 filter entries failed with -EOPNOTSUPP.
>> Eg. when using the following tc command, the FLOW_DISSECTOR_KEY_IPV6_ADDRS bit
>> is set in dissector->used_keys:
>> tc filter add dev p1 ingress protocol ip flower skip_sw dst_ip 224.0.1.100 \
>>     action trap
>>
>> To make ipv4 filter entries work i had to add FLOW_DISSECTOR_KEY_IPV6_ADDRS to
>> the allowed keys bitmask and check the addr_type instead.
>
>I see. But that sounds like a bug / silliness in the core that should
>be fixed. AFAICT it's due to the fact that the fields are a union and
>FL_KEY_SET_IF_MASKED() ends up interpreting either being set as both :/

Changes to the core would have to be done in a different patch submission? What
should be done with this patch, wait untill the core has been patched?

There could be drivers unknowingly depend on the current implementation of
having both bits always set.

thanks,
Cedric