tc flower IP address keys

I think there is a bug in net/sched/cls_flower.c:fl_init_dissector(),
 where it reads

    FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                 FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
    FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                 FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);

(and correspondingly further down for enc IP addrs), in that fl_flow_key
 stores ipv4 and ipv6 in a union, meaning that if a tc flower filter sets,
 for instance, a destination IPv4 address (and an ethertype of IPv4), the
 mask for this will also show up in the middle of the source ipv6 address,
 and lead to the ipv6 key being marked as used.  Thus subsequently a call
 to dissector_uses_key(..., FLOW_DISSECTOR_KEY_IPV6_ADDRS) will return
 true.
While this can be disambiguated by examining key->control.addr_type, this
 seems like it ought not to be necessary.  I apologise if this has been
 asked before, but is there a reason why the core code cannot fix this up
 and prevent drivers from seeing it in their tc offload routines?

-Ed

On Thu, Jan 10, 2019 at 5:42 AM Edward Cree [off-list ref] wrote:

I think there is a bug in net/sched/cls_flower.c:fl_init_dissector(),
 where it reads

    FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                 FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
    FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                 FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);

(and correspondingly further down for enc IP addrs), in that fl_flow_key
 stores ipv4 and ipv6 in a union, meaning that if a tc flower filter sets,
 for instance, a destination IPv4 address (and an ethertype of IPv4), the
 mask for this will also show up in the middle of the source ipv6 address,
 and lead to the ipv6 key being marked as used.  Thus subsequently a call
 to dissector_uses_key(..., FLOW_DISSECTOR_KEY_IPV6_ADDRS) will return
 true.
While this can be disambiguated by examining key->control.addr_type, this
 seems like it ought not to be necessary.  I apologise if this has been
 asked before, but is there a reason why the core code cannot fix this up
 and prevent drivers from seeing it in their tc offload routines?

I don't follow this question. The code you quoted from
fl_init_dissector() is correct, as it merely stores the offsets of ipv4/ipv6
fields. If somewhere we use it without checking control.addr_type,
then it is a bug there.

I don't see anything particular here, we use IPv4/IPv6 address union
in so many other places in networking, I don't see why this one is so
special to you. :)

Thanks.

On 11/01/19 00:31, Cong Wang wrote:

I don't follow this question. The code you quoted from
fl_init_dissector() is correct, as it merely stores the offsets of ipv4/ipv6
fields. If somewhere we use it without checking control.addr_type,
then it is a bug there.

My point is that it shouldn't store the offset of the ipv6 field if the
 ipv6 field isn't actually used.  I've been looking at driver implementations
 of tc flower offload, and it seems like the usual pattern is to use
 dissector_uses_key() to determine whether a field is being filtered on; then
 the drivers are having to break that pattern in the case of IP addrs and
 check this other bit of info in control.addr_type.
When initially creating the flow dissector from the tc filter netlink message,
 we have the information that the IPv4 address was specified and the IPv6
 wasn't.  But rather than putting that information in used_keys (like we do
 with _all_ the other information about "was this field specified"), we stuff
 it in this special other field.  Which is still maskable despite that not
 making sense, so everyone has to check its mask is all-ones too.
So I suppose it's not a bug (I was wrong to call it that), but it is a wart.
Why is it so hard for a flower filter to just not set an offset for IPV6_ADDRS
 when it's an IPv4 address filter?  I know that's not how it works right now,
 but _why_ not?

I don't see anything particular here, we use IPv4/IPv6 address union
in so many other places in networking, I don't see why this one is so
special to you. :)

Usually we don't also have a mask that's similarly unioned and is part of
 identifying whether a field is used.
Either the IP addresses are a single unified datum, in which case there should
 just be a single FLOW_DISSECTOR_KEY_IP_ADDRS, or they're separate, in which
 case the distinction between FLOW_DISSECTOR_KEY_IPV4_ADDRS and ..._IPV6_ADDRS
 in used_keys should be meaningful and informative.

-Ed