[ovs-dev] [PATCH] netdev-offload-tc: use nl_msg_put_flag for OVS_TUNNEL_KEY_ATTR_CSUM

[Eelco Chaudron]

On 7 Apr 2021, at 13:43, Paolo Valerio wrote:

> when a tunnel port gets added to the bridge setting the checksum 
> option
> to true:
>
> ovs-vsctl add-port br0 geneve0 -- set interface geneve0 type=geneve 
> options:remote_ip=<remote_ip> options:key=<key> options:csum=true
>
> the flow dump for the outgoing traffic will include a
> "bad key length 1 ..." message:
>
> ovs-appctl dpctl/dump-flows --names -m
> ufid:1bee60c5-53df-4c65-8e18-f042df400423, 
> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(ovs-l0),packet_type(ns=0/0,id=0/0),eth(src=8e:31:e4:89:03:30,dst=c2:be:66:7e:12:c7),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), 
> packets:4, bytes:336, used:0.930s, dp:tc, 
> actions:set(tunnel(tun_id=0x7b,dst=192.168.56.11,ttl=64,tp_dst=6081,key6(bad 
> key length 1, expected 0)(01)flags(key))),genev_sys_6081
>
> This is due to a mismatch present between the expected length (zero
> for OVS_TUNNEL_KEY_ATTR_CSUM in ovs_tun_key_attr_lens) and the
> current one.
>
> With this patch the same flow dump becomes:
>
> ovs-appctl dpctl/dump-flows --names -m
> ufid:b5f638c5-6a06-4ff5-8bbd-a16fc1d24844, 
> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(ovs-l0),packet_type(ns=0/0,id=0/0),eth(src=8e:31:e4:89:03:30,dst=c2:be:66:7e:12:c7),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), 
> packets:12, bytes:1008, used:0.510s, dp:tc, 
> actions:set(tunnel(tun_id=0x7b,dst=192.168.56.11,ttl=64,tp_dst=6081,flags(csum|key))),genev_sys_6081
>
> Fixes: d9677a1f0eaf ("netdev-tc-offloads: TC csum option is not 
> matched with tunnel configuration")
> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>

Looks good to me, as this is in line with how the kernel module handles 
this.

Acked-by: Eelco Chaudron <echaudro at redhat.com>