From patchwork Mon Aug 16 05:10:07 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony van der Peet X-Patchwork-Id: 1517016 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=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=alliedtelesis.co.nz header.i=@alliedtelesis.co.nz header.a=rsa-sha256 header.s=mail181024 header.b=RQ64ia/6; dkim-atps=neutral Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Gp2Ky2Nf9z9sW8 for ; Mon, 16 Aug 2021 15:10:30 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 91F5340398; Mon, 16 Aug 2021 05:10:27 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AovedO0kTkSp; Mon, 16 Aug 2021 05:10:24 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 114C94034B; Mon, 16 Aug 2021 05:10:23 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D8CECC0010; Mon, 16 Aug 2021 05:10:22 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id A6BD4C000E for ; Mon, 16 Aug 2021 05:10:21 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 838D440393 for ; Mon, 16 Aug 2021 05:10:21 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uFBd25AogkPJ for ; Mon, 16 Aug 2021 05:10:16 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from gate2.alliedtelesis.co.nz (gate2.alliedtelesis.co.nz [202.36.163.20]) by smtp4.osuosl.org (Postfix) with ESMTPS id E10124034B for ; Mon, 16 Aug 2021 05:10:15 +0000 (UTC) Received: from svr-chch-seg1.atlnz.lc (mmarshal3.atlnz.lc [10.32.18.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by gate2.alliedtelesis.co.nz (Postfix) with ESMTPS id 4631F806AC for ; Mon, 16 Aug 2021 17:10:10 +1200 (NZST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alliedtelesis.co.nz; s=mail181024; t=1629090610; bh=Vk/MOfjwOs5onRwfNExDF18/AYgOjoi/f6wwklEEZVQ=; h=From:To:Cc:Subject:Date; b=RQ64ia/6FLNmWfq7tBH/fwFeC/i2TM3E3UT+BsaM4SyS+y9vabLsMXI1JrgZyivv7 K4T1zeelQMNmL+khnugrPfSvUB8EUc1WwZmWZONQnM2axmXtvPf7lu8CMHYoWsfiJQ dEGj3cjavcSNjBescznV86czKBDwZ55Kqaqz25/BQUgq6mB9nDr8a5xRFYmT19bNfD D+xcvsIWNehA/JTUjCN+34Mn5+KZOQ9X1lFQ/wJAFtzTBHJjbdn/Sia3X0fhtU/N9f D1bEhJBLpXcRtbIYO5OrWehum2PMJiYqpVlqhLyJi1ETzYnsV2wkBjC6DMicM2gzxL MavfBnK07tQ3w== Received: from pat.atlnz.lc (Not Verified[10.32.16.33]) by svr-chch-seg1.atlnz.lc with Trustwave SEG (v8, 2, 6, 11305) id ; Mon, 16 Aug 2021 17:10:10 +1200 Received: from tonyp-dl.ws.atlnz.lc (tonyp-dl.ws.atlnz.lc [10.33.22.13]) by pat.atlnz.lc (Postfix) with ESMTP id 161BD13ECC4; Mon, 16 Aug 2021 17:10:10 +1200 (NZST) Received: by tonyp-dl.ws.atlnz.lc (Postfix, from userid 1605) id 11C223C28EC; Mon, 16 Aug 2021 17:10:10 +1200 (NZST) From: Tony van der Peet To: dev@openvswitch.org Date: Mon, 16 Aug 2021 17:10:07 +1200 Message-Id: <20210816051007.16373-1-tony.vanderpeet@alliedtelesis.co.nz> X-Mailer: git-send-email 2.32.0 MIME-Version: 1.0 X-SEG-SpamProfiler-Analysis: v=2.3 cv=aqTM9hRV c=1 sm=1 tr=0 a=KLBiSEs5mFS1a/PbTCJxuA==:117 a=MhDmnRu9jo8A:10 a=gjqHy4Tsx4SNoTqQSCEA:9 X-SEG-SpamProfiler-Score: 0 x-atlnz-ls: pat Cc: Tony van der Peet Subject: [ovs-dev] [PATCH] dpif-netdev: Fix crash when PACKET_OUT is metered 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" When a PACKET_OUT has output port of OFPP_TABLE, and the rule table includes a meter and this causes the packet to be deleted, execute with a clone of the packet, restoring the original packet if it is changed by the execution. Add tests to verify the original issue is fixed, and that the fix doesn't break tunnel processing. Reported-by: Tony van der Peet Signed-off-by: Tony van der Peet --- lib/dp-packet.h | 13 ++++++++++ lib/dpif-netdev.c | 19 +++++++++++++- tests/ofproto-dpif.at | 21 +++++++++++++++ tests/tunnel-push-pop.at | 56 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 1 deletion(-) diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 08d93c277..3dc582fbf 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -199,6 +199,7 @@ struct dp_packet *dp_packet_clone_data_with_headroom(const void *, size_t, void dp_packet_resize(struct dp_packet *b, size_t new_headroom, size_t new_tailroom); static inline void dp_packet_delete(struct dp_packet *); +static inline void dp_packet_swap(struct dp_packet *, struct dp_packet *); static inline void *dp_packet_at(const struct dp_packet *, size_t offset, size_t size); @@ -256,6 +257,18 @@ dp_packet_delete(struct dp_packet *b) } } +/* Swaps content of two packets. */ +static inline void +dp_packet_swap(struct dp_packet *a, struct dp_packet *b) +{ + ovs_assert(a->source == DPBUF_MALLOC || a->source == DPBUF_STUB); + ovs_assert(b->source == DPBUF_MALLOC || b->source == DPBUF_STUB); + struct dp_packet c = *a; + + *a = *b; + *b = c; +} + /* If 'b' contains at least 'offset + size' bytes of data, returns a pointer to * byte 'offset'. Otherwise, returns a null pointer. */ static inline void * diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 03f460c7d..4733c3a06 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4077,7 +4077,10 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) flow_hash_5tuple(execute->flow, 0)); } - dp_packet_batch_init_packet(&pp, execute->packet); + /* Making a copy because the packet might be stolen during the execution + * and caller might still need it. */ + struct dp_packet *packet_clone = dp_packet_clone(execute->packet); + dp_packet_batch_init_packet(&pp, packet_clone); dp_netdev_execute_actions(pmd, &pp, false, execute->flow, execute->actions, execute->actions_len); dp_netdev_pmd_flush_output_packets(pmd, true); @@ -4087,6 +4090,20 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) dp_netdev_pmd_unref(pmd); } + if (dp_packet_batch_size(&pp)) { + /* Packet wasn't dropped during the execution. Swapping content with + * the original packet, because the caller might expect actions to + * modify it. */ + dp_packet_swap(execute->packet, packet_clone); + dp_packet_delete_batch(&pp, true); + } else { + /* Packet was stolen during the execution due to some error. We need + * to flag that for the caller, so it will not proceed with other + * actions on this packet. Returning EAGAIN because we just don't + * know what execlty happened. */ + return EAGAIN; + } + return 0; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index ed3b141b3..70f9cb89f 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -9723,6 +9723,27 @@ OFPST_TABLE reply (OF1.3) (xid=0x2): OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif packet-out table meter drop]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 + +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop rate=1']) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,output:2']) + +ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000400080000 actions=resubmit(,0)" +ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000400080000 actions=resubmit(,0)" + +# Check that vswitchd hasn't crashed by dumping the meter added above +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip], [0], [dnl +OFPST_METER_CONFIG reply (OF1.3): +meter=1 pktps bands= +type=drop rate=1 +]) +OVS_WAIT_UNTIL([grep 'execute meter(0),2 failed' ovs-vswitchd.log]) + +OVS_VSWITCHD_STOP(["/execute meter(0),2 failed/d"]) +AT_CLEANUP + AT_SETUP([ofproto-dpif - ICMPv6]) OVS_VSWITCHD_START add_of_ports br0 1 diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index 48c5de9d1..12fc1ef91 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -595,6 +595,62 @@ OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep 50540000000a5054000000091235 | wc OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([tunnel_push_pop - packet_out debug_slow]) + +OVS_VSWITCHD_START( + [add-port br0 p0 dnl + -- set Interface p0 type=dummy ofport_request=1 dnl + other-config:hwaddr=aa:55:aa:55:00:00]) +AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg]) +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy]) +AT_CHECK([ovs-vsctl add-port int-br t2 dnl + -- set Interface t2 type=geneve options:remote_ip=1.1.2.92 dnl + options:key=123 ofport_request=2]) + +dnl First setup dummy interface IP address, then add the route +dnl so that tnl-port table can get valid IP address for the device. +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 1.1.2.88/24], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK +]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) + +dnl This ARP reply from p0 has two effects: +dnl 1. The ARP cache will learn that 1.1.2.92 is at f8:bc:12:44:34:b6. +dnl 2. The br0 mac learning will learn that f8:bc:12:44:34:b6 is on p0. +AT_CHECK([ + ovs-appctl netdev-dummy/receive p0 dnl + 'recirc_id(0),in_port(2),dnl + eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),dnl + arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)' +]) + +AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap]) + +packet=50540000000a505400000009123 +encap=f8bc124434b6aa55aa5500000800450000320000400040113406010102580101025c83a917c1001e00000000655800007b00 + +dnl Output to tunnel from a int-br internal port. +dnl Checking that the packet arrived and it was correctly encapsulated. +AT_CHECK([ovs-ofctl add-flow int-br "in_port=LOCAL,actions=debug_slow,output:2"]) +AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"]) +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l` -ge 1]) +dnl Sending again to exercise the non-miss upcall path. +AT_CHECK([ovs-appctl netdev-dummy/receive int-br "${packet}4"]) +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}4" | wc -l` -ge 2]) + +dnl Output to tunnel from the controller. +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out int-br CONTROLLER "debug_slow,output:2" "${packet}5"]) +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep "${encap}${packet}5" | wc -l` -ge 1]) + +dnl Datapath actions should not have tunnel push action. +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -q tnl_push], [1]) +dnl There should be slow_path action instead. +AT_CHECK([ovs-appctl dpctl/dump-flows | grep -q 'slow_path(action)'], [0]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([tunnel_push_pop - underlay bridge match]) OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00])