From 38efd803dd100d46219452c6c1ef5e6dd3716fbe Mon Sep 17 00:00:00 2001 From: Tony van der Peet Date: Mon, 16 Aug 2021 17:10:07 +1200 Subject: [PATCH] dpif-netdev: Fix crash when PACKET_OUT is metered. 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 Signed-off-by: Ilya Maximets --- lib/dp-packet.h | 13 ++++++++++ lib/dpif-netdev.c | 13 +++++++++- tests/ofproto-dpif.at | 20 ++++++++++++++ tests/tunnel-push-pop.at | 56 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/lib/dp-packet.h b/lib/dp-packet.h index dc8b773fe1b..4c14951d6ac 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -155,6 +155,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); @@ -212,6 +213,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 f60b9c2c2ce..1be789433f0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3929,7 +3929,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); @@ -3939,6 +3942,14 @@ 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); + } + return 0; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 6415a8a04df..7b5b2183bf3 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -9268,6 +9268,26 @@ 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_VSWITCHD_STOP +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 b92c23fde8f..9feb1c5fec9 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -573,6 +573,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])