When IPF detects a fragment that overlaps a previously received fragment
of the same datagram, discard not just that fragment but the entire
fragment list it belongs to. Per RFC 5722 and RFC 8200, when an
overlapping fragment is detected the whole datagram (and all of its
constituent fragments) must be silently discarded.
Previously such a fragment had its CT state marked invalid and was
returned to the conntrack batch, leaving the rest of the (now
unreassemblable) fragment list pinned until expiry. Instead, delete the
overlapping packet and tear down the whole fragment list, which also
avoids the work conntrack would otherwise do to mark the fragments as
invalid.
Assisted-by: composer-2.5-fast, Cursor
Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
Signed-off-by: Eli Britstein <[email protected]>
---
lib/ipf.c | 28 ++++++++++++++++++-----
tests/ofproto-dpif.at | 52 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+), 6 deletions(-)
diff --git a/lib/ipf.c b/lib/ipf.c
index d836b8824..ab4c02c83 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -822,10 +822,26 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int
last_inuse_idx,
return false;
}
-/* Adds a fragment to a list of fragments, if the fragment is not a
- * duplicate. If the fragment is a duplicate, that fragment is marked
- * invalid to avoid the work that conntrack would do to mark the fragment
- * as invalid, which it will in all cases. */
+/* Drops the entire fragment list: deletes every fragment packet held by the
+ * list and removes the list from the tracking datastructures. Per RFC 5722
+ * and RFC 8200, when an overlapping fragment is detected the whole datagram
+ * and all of its already received fragments must be silently discarded. */
+static void
+ipf_drop_frag_chain(struct ipf *ipf, struct ipf_list *ipf_list)
+ OVS_REQUIRES(ipf->ipf_lock)
+{
+ for (int i = 0; i <= ipf_list->last_inuse_idx; i++) {
+ dp_packet_delete(ipf_list->frag_list[i].pkt);
+ atomic_count_dec(&ipf->nfrag);
+ }
+
+ ipf_list_clean(&ipf->frag_lists, ipf_list);
+}
+
+/* Adds a fragment to a list of fragments, if the fragment does not overlap
+ * an existing fragment. If it overlaps, the whole fragment list is dropped.
+ * (see ipf_drop_frag_chain()), avoiding the work that conntrack would
+ * otherwise do to mark the fragments as invalid. */
static bool
ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
struct dp_packet *pkt, uint16_t start_data_byte,
@@ -852,8 +868,8 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
}
} else {
ipf_count(ipf, v6, IPF_NFRAGS_OVERLAP);
- pkt->md.ct_state = CS_INVALID;
- return false;
+ dp_packet_delete(pkt);
+ ipf_drop_frag_chain(ipf, ipf_list);
}
return true;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index efb36e058..53437dd06 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5669,6 +5669,58 @@ CHECK_COVERAGE([dpif_netdev_output_grow_queues], [1])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - fragment handling - drop duplicate fragment])
+OVS_VSWITCHD_START
+add_of_ports --pcap br0 1 90
+
+AT_DATA([flows.txt], [dnl
+table=0 in_port=90,ip actions=ct(commit),output:1
+])
+AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
+
+dnl Send the same fragment twice. It is a last fragment (MF=0) at offset 400,
+dnl so it bypasses the minimum fragment size check and is admitted, yet it
+dnl cannot complete reassembly on its own (bytes 0..399 are missing). The
+dnl second copy is a duplicate and must be dropped, rather than marked CT
+dnl invalid and returned to the datapath, so it is never forwarded.
+dnl
+dnl Last fragment carrying bytes 400..799 (MF clear).
+dnl Ethernet II, Src: 50:54:00:00:00:09, Dst: 50:54:00:00:00:0a
+dnl Type: IPv4 (0x0800)
+dnl Internet Protocol Version 4, Src: 10.1.1.1, Dst: 10.1.1.2
+dnl 0100 .... = Version: 4
+dnl .... 0101 = Header Length: 20 bytes (5)
+dnl Differentiated Services Field: 0x00 (DSCP: CS0, ECN: Not-ECT)
+dnl Total Length: 420
+dnl Identification: 0x0020 (32)
+dnl 000. .... = Flags: 0x0 (Last fragment)
+dnl ...0 0000 0011 0010 = Fragment Offset: 400
+dnl Time to Live: 64
+dnl Protocol: UDP (17)
+dnl Header Checksum: 0x62f3
+dnl Data (400 bytes)
+eth="50 54 00 00 00 0a 50 54 00 00 00 09 08 00"
+ip="45 00 01 a4 00 20 00 32 40 11 62 f3"
+addrs="0a 01 01 01 0a 01 01 02"
+data=$(printf '%0*d' 800 0)
+packet="${eth}${ip}${addrs}${data}"
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet"])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet"])
+
+AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
+| grep -E 'v4 frags accepted:|v4 frags overlapped:'], [], [dnl
+ v4 frags accepted: 1
+ v4 frags overlapped: 1
+])
+
+dnl The duplicate fragment is dropped, not marked invalid and forwarded, so
+dnl nothing must be transmitted on the output port.
+AT_CHECK([test 0 = `ovs-ofctl parse-pcap p1-tx.pcap | wc -l`])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
OVS_VSWITCHD_START
add_of_ports br0 1 90
--
2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
