[ovs-discuss] Patch for PACKET_OUT getting deleted twice crash

[Tony van der Peet]

Here is a patch with both a test and a fix. Not submitting as a formal
patch because I would like some feedback on whether 1) maintainers feel
this is worth fixing and 2) whether this is the way to fix it.

I have tried to make the most minimal change possible, but this means that
there might be paths through the code that give unexpected behaviour (which
in the worst case would be a memory leak I suppose).

Tony

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 246be14d0..5e0dabe67 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -739,6 +739,7 @@ struct dp_packet_batch {
    size_t count;
    bool trunc; /* true if the batch needs truncate. */
    bool do_not_steal; /* Indicate that the packets should not be stolen.
*/
+    bool packet_out; /* Indicate single packet is PACKET_OUT */
    struct dp_packet *packets[NETDEV_MAX_BURST];
};

@@ -748,6 +749,7 @@ dp_packet_batch_init(struct dp_packet_batch *batch)
    batch->count = 0;
    batch->trunc = false;
    batch->do_not_steal = false;
+    batch->packet_out = false;
}

static inline void
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 650e67ab3..deba4a94a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4170,6 +4170,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
dpif_execute *execute)

    dp_packet_batch_init_packet(&pp, execute->packet);
    pp.do_not_steal = true;
+    pp.packet_out = execute->packet_out;
    dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
                              execute->actions, execute->actions_len);
    dp_netdev_pmd_flush_output_packets(pmd, true);
@@ -6170,6 +6171,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
dp_packet_batch *packets_,
    int exceeded_band[NETDEV_MAX_BURST];
    uint32_t exceeded_rate[NETDEV_MAX_BURST];
    int exceeded_pkt = cnt; /* First packet that exceeded a band rate. */
+    bool do_not_delete = packets_->count == 1 && packets_->packet_out;

    if (meter_id >= MAX_METERS) {
        return;
@@ -6295,7 +6297,9 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct
dp_packet_batch *packets_,
            band->packet_count += 1;
            band->byte_count += dp_packet_size(packet);
            COVERAGE_INC(datapath_drop_meter);
-            dp_packet_delete(packet);
+            if (!do_not_delete) {
+                dp_packet_delete(packet);
+            }
        } else {
            /* Meter accepts packet. */
            dp_packet_batch_refill(packets_, packet, j);
diff --git a/lib/dpif.h b/lib/dpif.h
index f9728e673..48415f6df 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -725,6 +725,7 @@ struct dpif_execute {
    size_t actions_len;             /* Length of 'actions' in bytes. */
    bool needs_help;
    bool probe;                     /* Suppress error messages. */
+    bool packet_out;                /* This is a PACKET_OUT */
    unsigned int mtu;               /* Maximum transmission unit to
fragment.
                                       0 if not a fragmented packet */
    uint64_t hash;                  /* Packet flow hash. 0 if not
specified. */
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ccf97266c..a6543ce2b 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1606,6 +1606,7 @@ handle_upcalls(struct udpif *udpif, struct upcall
*upcalls,
            op->dop.execute.probe = false;
            op->dop.execute.mtu = upcall->mru;
            op->dop.execute.hash = upcall->hash;
+            op->dop.execute.packet_out = false;
        }
    }

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 47db9bb57..6d03234c5 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4949,6 +4949,7 @@ packet_execute_prepare(struct ofproto *ofproto_,
    execute->needs_help = aux->needs_help;
    execute->probe = false;
    execute->mtu = 0;
+    execute->packet_out = true;

    /* Fix up in_port. */
    ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31064ed95..522cc1318 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -9520,6 +9520,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=50540000000a50540000000908004500001c000000000011a4cd0a01
01010a0101020001000400080000 actions=resubmit(,0)"
+ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
packet=50540000000a50540000000908004500001c000000000011a4cd0a01
01010a0101020001000400080000 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-discuss/attachments/20210604/9a1c854d/attachment.html>