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>
---
lib/netdev-offload-tc.c | 4 +++-
lib/tc.h | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index b846a63c2..164c7eef6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
}
break;
case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
- action->encap.tp_src = nl_attr_get_be16(tun_attr);
+ /* There is no corresponding attribute in TC. */
+ VLOG_DBG_RL(&rl, "unsupported tunnel key attribute TP_SRC");
+ return EOPNOTSUPP;
}
break;
case OVS_TUNNEL_KEY_ATTR_TP_DST: {
diff --git a/lib/tc.h b/lib/tc.h
index 06707ffa4..fdbcf4b7c 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -213,7 +213,8 @@ enum nat_type {
struct tc_action_encap {
bool id_present;
ovs_be64 id;
- ovs_be16 tp_src;
+ /* ovs_be16 tp_src; Could have been here, but there is no
+ * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
ovs_be16 tp_dst;
uint8_t tos;
uint8_t ttl;
--
2.41.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
