On 30 Oct 2023, at 15:00, Ilya Maximets wrote:
> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
> by OVS. Current code just ignores the attribute in the tunnel(set())
> action leading to a flow mismatch and potential incorrect datapath
> behavior:
>
> |tc(handler21)|DBG|tc flower compare failed action compare
> ...
> Action 0 mismatch:
> - Expected Action:
> 00000010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
> 00000020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
> 00000050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00000060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
> ...
> - Received Action:
> 00000010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
> 00000020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
> 00000050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00000060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
> ...
>
> In the tc_action dump above we can see the difference on the second
> line. The action dumped from a kernel is missing 'c0 5b' - source
> port for a tunnel(set()) action on the second line.
>
> Removing the field from the tc_action_encap structure entirely to
> avoid any potential confusion.
>
> Note: In general, the source port number in the tunnel(set()) action
> is not particularly useful for most tunnels, because they may just
> ignore the value. Specs for Geneve and VXLAN suggest using a value
> based on the headers of the inner packet as a source port.
> And I'm not really sure how to make OVS to generate the action with
> a source port in it, so the commit doesn't include the test case.
> In vast majority of scenarios the source port doesn't actually end
> up in the action itself.
> Having a mismatch between the userspace and TC leads to constant
> revalidation of the flow and warnings in the log, and might
> potentially cause mishandling of the traffic, even though the impact
> is unclear at the moment.
>
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
> interface")
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
> Reported-by: Vladislav Odintsov <odiv...@gmail.com>
> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
Thanks Ilya, this change seems correct as the kernel does not seem to support
setting the source port. I did some additional tests, and found no problems.
Acked-by: Eelco Chaudron <echau...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
