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
