From patchwork Thu Aug 20 22:45:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yi-Hung Wei X-Patchwork-Id: 1348642 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=lvTN5dFY; dkim-atps=neutral Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BXfr46lmSz9sPB for ; Fri, 21 Aug 2020 08:45:40 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id AD96E86B7A; Thu, 20 Aug 2020 22:45:38 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id F_r4ljsl30Mj; Thu, 20 Aug 2020 22:45:36 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 6D6C286B89; Thu, 20 Aug 2020 22:45:36 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4269DC0889; Thu, 20 Aug 2020 22:45:33 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id D5E62C0051 for ; Thu, 20 Aug 2020 22:45:31 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id CB4C4883BE for ; Thu, 20 Aug 2020 22:45:31 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2pzY5rVnTHgP for ; Thu, 20 Aug 2020 22:45:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-pf1-f194.google.com (mail-pf1-f194.google.com [209.85.210.194]) by whitealder.osuosl.org (Postfix) with ESMTPS id CCEE4882EC for ; Thu, 20 Aug 2020 22:45:30 +0000 (UTC) Received: by mail-pf1-f194.google.com with SMTP id m8so95154pfh.3 for ; Thu, 20 Aug 2020 15:45:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=LWP9KU7+CzbD8fy/TxzXlLCwDvUFZ1QUNRYYhy24cXU=; b=lvTN5dFYr4vSvBReJXAVEhFeFOifXdMwZrelxtXoLB0Zo2U6BsmTDU9yvjRgI1uIUi lbDQIbKkxflEIsqUpgJPaOdjytrv8S0z6SaKiJIcsK1jS4PpJylwj7QbNuKCJLEnpvbm yAvlgTh1xBIeDHJZo5EOB4mgcil+EjZ2pDnM0QQV5kwqU7BM7MkhLvSarlY0GozUjnDh SyBPaf9WsF9AxWYgO+g0SwjhLkaOHD+uPwho8LKaGOiYC9eAklkBoXHO/lSTQk6GmYMe 7g77/AXf1yLSAkkywpaR1HVenK4yyQHiBBOGWwPPUKkeDo7kmGAiGCYSAmSpou0qyo7f ttxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=LWP9KU7+CzbD8fy/TxzXlLCwDvUFZ1QUNRYYhy24cXU=; b=JuOqvhHsW4ghYezE0XhP0s5p8lgrTsrQzvQRJu6OfaKDI37h6h2GhTjlDU89QHrbN8 KaG7/6lP6/9Fqdy79EJ/hcV3p9OduvjmIMt/oXhTmCsKlOcpoWaEtLzM0sjiFhKCc8SB MbYUtk3l6YyGZZKu8hwjf1q68GQ9CcwDHcwDYmoJkAusvI+6L7LWwSnFRhi94YihRILt MWMC6+2ONN6nLFh8NVC9Qe+pM8jpdIAc4git/mcf80q8Bw7PRtakHh4MmruPIifLzrU7 UTOHii8ttZ4CMkTRgn03bLFkeDr4TxK7rpbY98VFu3fI+m+3IN9bRXbB/NHBR36L56zg 7phA== X-Gm-Message-State: AOAM532Gz3800652+WR2Am1rKelF0PcRiBCXaWRc0YZaw/MsJYU8OiN8 8ufKpb1CT1LpcAe0vp+T++0pGaUX1yFuEA== X-Google-Smtp-Source: ABdhPJyqkgf52Iy5iz3lqhzBo7k3NtSzCG9SkV6nVG8uulEeMeoVI1+JomwLoUlfm8E+dxRr5WQzWA== X-Received: by 2002:a63:7a06:: with SMTP id v6mr195791pgc.441.1597963529605; Thu, 20 Aug 2020 15:45:29 -0700 (PDT) Received: from vm-main.eng.vmware.com ([66.170.99.2]) by smtp.gmail.com with ESMTPSA id o14sm36213pjw.11.2020.08.20.15.45.27 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 20 Aug 2020 15:45:28 -0700 (PDT) From: Yi-Hung Wei To: dev@openvswitch.org Date: Thu, 20 Aug 2020 15:45:22 -0700 Message-Id: <1597963522-30983-1-git-send-email-yihung.wei@gmail.com> X-Mailer: git-send-email 2.7.4 Subject: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix tun_metadata match after recirc 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Consider the following OpenFlow rules that match on tun_metadata0 after recirculation. If we start ICMP flow with tun_metadata0=0x1 follow by a flow with tun_metadata0=0x3, OVS will incorrectly match the second flow with the tun_metadata0=0x1 rule. table=0, in_port=gnv1, icmp, action=ct(table=1) table=0, in_port=gnv0, icmp action=ct(table=1) table=1, in_port=gnv1, icmp, action=output:gnv0 table=1, in_port=gnv0, icmp, ct_state=+trk+new, action=ct(commit),output:gnv1 table=1, in_port=gnv0, icmp, ct_state=+trk+est, tun_metadata0=0x1 action=output:gnv1 table=1, in_port=gnv0, icmp, ct_state=+trk+est, tun_metadata0=0x3 action=output:gnv1 The root cause of this issue is that during recirculation OVS will overwrite the tun_metadata0 with the one stored in the frozen state. This will result in erroneous behavior if tun_metadata0 is wildcarded in the megaflow as shown below. Notice that both the second and the third megaflow both carry 0x1 in tun_metadata0, where the expected behavior in the thrid megaflow one should be 0x3. recirc_id(0),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100,geneve({}{}{}), flags(-df-csum+key)),in_port(5),eth(),eth_type(0x0800),ipv4(proto=1,frag=no), packets:213, bytes:20774, used:0.672s, actions:ct,recirc(0x4) recirc_id(0x4),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100, geneve({class=0x102,type=0x7,len=4,0x1}{class=0,type=0,len=0}), flags(-df-csum+key)),in_port(5),ct_state(-new+est+trk),eth(),eth_type(0x0800), ipv4(proto=1,tos=0/0x3,frag=no), packets:112, bytes:10976, used:0.672s, actions:set(tunnel(tun_id=0x2,dst=172.31.1.2,ttl=64,tp_src=64725,tp_dst=6081, geneve({class=0x102,type=0x7,len=4,0x1}),flags(df|key))),5 recirc_id(0x4),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100, geneve({class=0x102,type=0x7,len=4,0x3}{class=0,type=0,len=0}), flags(-df-csum+key)),in_port(5),ct_state(-new+est+trk),eth(),eth_type(0x0800), ipv4(proto=1,tos=0/0x3,frag=no), packets:98, bytes:9506, used:0.992s, actions:set(tunnel(tun_id=0x2,dst=172.31.1.2,ttl=64,tp_src=64725,tp_dst=6081, geneve({class=0x102,type=0x7,len=4,0x1}),flags(df|key))),5 There seems to be two ways to fix this issue, one is to unwildcarded all the tunnel metadata when OVS generates megaflow with recirculation. The other one is to add a boolean flag in the frozen state, set the flag if the flow is from a tunnel port, and ask OVS to honor the tunnel metadata from the flow instead of from the frozen state. Since the first approach will increase the number of megaflows and may incur performance impact, this patch takes the second approach. With this patch, the megaflows become as below: recirc_id(0),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100, geneve({}{}{}),flags(-df-csum+key)),in_port(5),eth(),eth_type(0x0800), ipv4(proto=1,frag=no), packets:60, bytes:5850, used:0.004s, actions:ct,recirc(0x8) recirc_id(0x8),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100, geneve({class=0x102,type=0x7,len=4,0x1}{class=0,type=0,len=0}), flags(-df-csum+key)),in_port(5),ct_state(-new+est+trk),eth(),eth_type(0x0800), ipv4(proto=1,tos=0/0x3,frag=no), packets:29, bytes:2842, used:0.356s, actions:set(tunnel(tun_id=0x2,dst=172.31.1.2,ttl=64,tp_src=64725,tp_dst=6081, geneve({class=0x102,type=0x7,len=4,0x1}),flags(df|key))),5 recirc_id(0x8),tunnel(tun_id=0x2,src=172.31.1.1,dst=172.31.1.100, geneve({class=0x102,type=0x7,len=4,0x3}{class=0,type=0,len=0}), flags(-df-csum+key)),in_port(5),ct_state(-new+est+trk), eth(),eth_type(0x0800), ipv4(proto=1,tos=0/0x3,frag=no), packets:28, bytes:2716, used:0.004s, actions:set(tunnel(tun_id=0x2,dst=172.31.1.2,ttl=64,tp_src=64725,tp_dst=6081, geneve({class=0x102,type=0x7,len=4,0x3}),flags(df|key))),5 VMWare-BZ: 2617696 Reported-by: Ran Gu Signed-off-by: Yi-Hung Wei --- https://travis-ci.org/github/YiHungWei/ovs/builds/719445339 --- ofproto/ofproto-dpif-rid.h | 8 ++++++-- ofproto/ofproto-dpif-upcall.c | 16 +++++++++++++++- ofproto/ofproto-dpif-xlate.c | 16 +++++++++++++++- tests/tunnel.at | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 4 deletions(-) diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index 30cd5275f24c..d6df1212cc0f 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -131,9 +131,12 @@ frozen_metadata_from_flow(struct frozen_metadata *md, static inline void frozen_metadata_to_flow(struct ofproto *ofproto, const struct frozen_metadata *md, - struct flow *flow) + struct flow *flow, + bool from_tunnel) { - flow->tunnel = md->tunnel; + if (!from_tunnel) { + flow->tunnel = md->tunnel; + } flow->tunnel.metadata.tab = ofproto_get_tun_tab(ofproto); flow->metadata = md->metadata; memcpy(flow->regs, md->regs, sizeof flow->regs); @@ -153,6 +156,7 @@ struct frozen_state { size_t stack_size; mirror_mask_t mirrors; /* Mirrors already output. */ bool conntracked; /* Conntrack occurred prior to freeze. */ + bool from_tunnel; /* Flow is from a tunnel port. */ bool was_mpls; /* MPLS packet */ struct uuid xport_uuid; /* UUID of 1st port packet received on. */ diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 72a5b4d734f0..da56e2afeb3c 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1536,8 +1536,22 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, flow_clear_conntrack(&frozen_flow); } + /* Packet-in message expects tunnel metadata in non-udpif + * format. */ + if (flow->tunnel.flags & FLOW_TNL_F_UDPIF) { + const struct tun_table *tun_tab = + ofproto_get_tun_tab(&upcall->ofproto->up); + int err = + tun_metadata_from_geneve_udpif(tun_tab, &flow->tunnel, + &flow->tunnel, + &frozen_flow.tunnel); + if (err) { + return err; + } + } + frozen_metadata_to_flow(&upcall->ofproto->up, &state->metadata, - &frozen_flow); + &frozen_flow, state->from_tunnel); flow_get_metadata(&frozen_flow, &am->pin.up.base.flow_metadata); ofproto_dpif_send_async_msg(upcall->ofproto, am); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e0ede2cab3ad..97c17d4f95c0 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -396,6 +396,11 @@ struct xlate_ctx { * state from the datapath should be honored after thawing. */ bool conntracked; + /* True if a packet is form a tunnel port. This is used to determine + * whether tunnel information from the datapath should be honored after + * thawing. */ + bool from_tunnel; + /* Pointer to an embedded NAT action in a conntrack action, or NULL. */ struct ofpact_nat *ct_nat_action; @@ -4858,6 +4863,7 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, .stack_size = ctx->stack.size, .mirrors = ctx->mirrors, .conntracked = ctx->conntracked, + .from_tunnel = ctx->from_tunnel, .was_mpls = ctx->was_mpls, .ofpacts = NULL, .ofpacts_len = 0, @@ -4933,6 +4939,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) .stack_size = ctx->stack.size, .mirrors = ctx->mirrors, .conntracked = ctx->conntracked, + .from_tunnel = ctx->from_tunnel, .was_mpls = ctx->was_mpls, .xport_uuid = ctx->xin->xport_uuid, .ofpacts = ctx->frozen_actions.data, @@ -7513,6 +7520,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) .was_mpls = false, .conntracked = false, + .from_tunnel = false, .ct_nat_action = NULL, @@ -7580,7 +7588,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) /* Restore pipeline metadata. May change flow's in_port and other * metadata to the values that existed when freezing was triggered. */ frozen_metadata_to_flow(&ctx.xbridge->ofproto->up, - &state->metadata, flow); + &state->metadata, flow, + state->from_tunnel); /* Restore stack, if any. */ if (state->stack) { @@ -7649,6 +7658,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) ctx.xin->xport_uuid = in_port->uuid; } + if (in_port && in_port->is_tunnel) { + ctx.from_tunnel = true; + } + if (flow->packet_type != htonl(PT_ETH) && in_port && in_port->pt_mode == NETDEV_PT_LEGACY_L3 && ctx.table_id == 0) { /* Add dummy Ethernet header to non-L2 packet if it's coming from a @@ -7917,6 +7930,7 @@ xlate_resume(struct ofproto_dpif *ofproto, .mirrors = pin->mirrors, .conntracked = pin->conntracked, .xport_uuid = UUID_ZERO, + .from_tunnel = false, /* When there are no actions, xlate_actions() will search the flow * table. We don't want it to do that (we want it to resume), so diff --git a/tests/tunnel.at b/tests/tunnel.at index e08fd1e048f8..e15ceaf61c56 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -949,6 +949,39 @@ Datapath actions: set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=6081,geneve({class=0xffff OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([tunnel - Match tun_metadata after recirc]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=geneve \ + options:remote_ip=1.1.1.1 ofport_request=1 \ + -- add-port br0 p2 -- set Interface p2 type=dummy \ + ofport_request=2 ofport_request=2]) +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP + +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=4}->tun_metadata0"]) + +AT_DATA([flows.txt], [dnl +table=0,udp,actions=ct(table=1) +table=1,udp,tun_metadata0=0x11,actions=IN_PORT +table=1,udp,tun_metadata0=0x22,actions=drop +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +dnl check flow with tun_metadata0=0x11 +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(0),tunnel(tun_id=0x0,src=1.1.1.1,dst=1.1.1.2,ttl=64,geneve({class=0xffff,type=0,len=4,0x11}),flags(df|key)),in_port(6081),skb_mark(0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.1.1,dst=192.168.1.2,proto=17,tos=0,ttl=64,frag=no),udp(src=1234,dst=5678)' -generate], [0], [stdout]) +AT_CHECK([tail -2 stdout], [0], + [Megaflow: recirc_id=0x1,eth,udp,tun_id=0,tun_src=1.1.1.1,tun_dst=1.1.1.2,tun_tos=0,tun_flags=+df-csum+key,tun_metadata0=0x11,in_port=1,nw_ecn=0,nw_frag=no +Datapath actions: set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=6081,geneve({class=0xffff,type=0,len=4,0x11}),flags(df))),6081 +]) + +dnl check flow with tun_metadata0=0x22 and recirc_id(0x1) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(0x1),tunnel(tun_id=0x0,src=1.1.1.1,dst=1.1.1.2,ttl=64,geneve({class=0xffff,type=0,len=4,0x22}),flags(df|key)),in_port(6081),skb_mark(0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.1.1,dst=192.168.1.2,proto=17,tos=0,ttl=64,frag=no),udp(src=1234,dst=5678)'], [0], [stdout]) +AT_CHECK([tail -2 stdout], [0], + [Megaflow: recirc_id=0x1,eth,udp,tun_id=0,tun_src=1.1.1.1,tun_dst=1.1.1.2,tun_tos=0,tun_flags=+df-csum+key,tun_metadata0=0x22,in_port=1,nw_frag=no +Datapath actions: drop +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([tunnel - concomitant IPv6 and IPv4 tunnels]) OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ options:remote_ip=1.1.1.1 ofport_request=1 \