From patchwork Mon Oct 30 14:00:29 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1857118 X-Patchwork-Delegate: i.maximets@samsung.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=patchwork.ozlabs.org) Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4SJvyv4k4Tz1yQW for ; Tue, 31 Oct 2023 00:59:35 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 33C36437F0; Mon, 30 Oct 2023 13:59:33 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 33C36437F0 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Sh9cei6Agjht; Mon, 30 Oct 2023 13:59:32 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp2.osuosl.org (Postfix) with ESMTPS id F3B4D4379B; Mon, 30 Oct 2023 13:59:30 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org F3B4D4379B Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C91A9C0071; Mon, 30 Oct 2023 13:59:30 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 09154C0032 for ; Mon, 30 Oct 2023 13:59:29 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id C86CB43790 for ; Mon, 30 Oct 2023 13:59:28 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org C86CB43790 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lrDX0A2hGbdG for ; Mon, 30 Oct 2023 13:59:27 +0000 (UTC) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::222]) by smtp2.osuosl.org (Postfix) with ESMTPS id 27CFD4330E for ; Mon, 30 Oct 2023 13:59:26 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 27CFD4330E Received: by mail.gandi.net (Postfix) with ESMTPSA id BA43140004; Mon, 30 Oct 2023 13:59:23 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Mon, 30 Oct 2023 15:00:29 +0100 Message-ID: <20231030140031.75157-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 X-GND-Sasl: i.maximets@ovn.org Cc: Vladislav Odintsov , Ilya Maximets Subject: [ovs-dev] [PATCH] netdev-offload-tc: Fix offload of tunnel key tp_src. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 Signed-off-by: Ilya Maximets Acked-by: Eelco Chaudron --- 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;