[ovs-dev] [PATCH 2/2] dpif-netlink: For non-Ethernet, use Ethertype from packet_type.

[Joe Stringer]

On 19 July 2017 at 08:56, Eric Garver <e at erig.me> wrote:
> On Tue, Jul 18, 2017 at 03:32:44PM -0700, Joe Stringer wrote:
>> For non-Ethernet flows, when fixing up the netlink message we need make
>> sure to pass down a valid Ethertype. The kernel does not understand
>> packet_type so it's implicitly encoded by the absence of _ETHERNET and
>> exact match of _ETHERTYPE. Without this change match_validate in the
>> kernel complains when trying to match packets from L3 tunnels.
>> e.g.
>>   openvswitch: netlink: Unexpected mask (mask=110088, allowed=3d9804c)
>>
>> The mask use to always be set in xlate_wc_init() and xlate_wc_finish(),
>> but that changed for non-Ethernet frames with the commit listed below.
>>
>> Fixes: 3d4b2e6eb74e ("userspace: Add OXM field MFF_PACKET_TYPE")
>> Signed-off-by: Joe Stringer <joe at ovn.org>
>> Co-authored-by: Eric Garver <e at erig.me>
>> ---
>>  lib/dpif-netlink.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 63ef3de5c7d1..55effd1f91a8 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -3434,8 +3434,9 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow,
>>
>>
>>  /*
>> - * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out,
>> - * then puts 'data' to 'buf'.
>> + * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out.
>> + * If the flow is not Ethernet, the OVS_KEY_ATTR_PACKET_TYPE is converted to
>> + * OVS_KEY_ATTR_ETHERTYPE. Puts 'data' to 'buf'.
>>   */
>>  static void
>>  put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
>> @@ -3458,6 +3459,20 @@ put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
>>          ofs = nl_msg_start_nested(buf, type);
>>          nl_msg_put(buf, data, first_chunk_size);
>>          nl_msg_put(buf, next_attr, second_chunk_size);
>> +        if (!nl_attr_find__(data, data_len, OVS_KEY_ATTR_ETHERNET)) {
>> +            ovs_be16 pt = pt_ns_type_be(nl_attr_get_be32(packet_type));
>> +            const struct nlattr *nla;
>> +
>> +            nla = nl_attr_find(buf, NLA_HDRLEN, OVS_KEY_ATTR_ETHERTYPE);
>> +            if (nla) {
>> +                ovs_be16 *ethertype;
>> +
>> +                ethertype = CONST_CAST(ovs_be16 *, nl_attr_get(nla));
>> +                *ethertype = pt;
>> +            } else {
>> +                nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, pt);
>> +            }
>> +        }
>>          nl_msg_end_nested(buf, ofs);
>>      } else {
>>          nl_msg_put_unspec(buf, type, data, data_len);
>> --
>> 2.11.1
>>
>
> Thanks Joe! This is definitely more concise than my version.
>
> Acked-by: Eric Garver <e at erig.me>

Thanks for the review, applied to master.