The previous check only tested whether a new fragment's endpoints fell
inside an existing range and treated any such match as a fragment to be
dropped on its own. This missed the case where a new fragment fully
contains an existing one, and it conflated two cases that the kernel and
RFCs 815, 5722 and 8200 handle differently:
- An exact duplicate (same byte range as an existing fragment) may be
dropped on its own while the rest of the fragment list is kept for
later reassembly (RFC 8200).
- An overlapping (but not duplicate) fragment means the whole datagram
must be silently discarded along with all of its fragments
(RFC 5722 / RFC 8200).
Detect these two cases separately using closed-interval overlap, and count
duplicates, as a pre-step towards handling real overlaps and duplicates
differently.
Assisted-by: composer-2.5-fast, Cursor
Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
Signed-off-by: Eli Britstein <[email protected]>
---
lib/dpctl.c | 4 ++
lib/dpif-provider.h | 1 +
lib/ipf.c | 52 +++++++++++++++++++++-----
lib/ipf.h | 1 +
tests/ofproto-dpif.at | 63 +++++++++++++++++++++++++++++++-
tests/system-userspace-macros.at | 8 ++++
6 files changed, 118 insertions(+), 11 deletions(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index d6ff348e5..c511b966c 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2627,6 +2627,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
dpif_ipf_status.v4.nfrag_too_small);
dpctl_print(dpctl_p, " v4 frags overlapped: %"PRIu64"\n",
dpif_ipf_status.v4.nfrag_overlap);
+ dpctl_print(dpctl_p, " v4 frags duplicate: %"PRIu64"\n",
+ dpif_ipf_status.v4.nfrag_duplicate);
dpctl_print(dpctl_p, " v4 frags purged: %"PRIu64"\n",
dpif_ipf_status.v4.nfrag_purged);
@@ -2642,6 +2644,8 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
dpif_ipf_status.v6.nfrag_too_small);
dpctl_print(dpctl_p, " v6 frags overlapped: %"PRIu64"\n",
dpif_ipf_status.v6.nfrag_overlap);
+ dpctl_print(dpctl_p, " v6 frags duplicate: %"PRIu64"\n",
+ dpif_ipf_status.v6.nfrag_duplicate);
dpctl_print(dpctl_p, " v6 frags purged: %"PRIu64"\n",
dpif_ipf_status.v6.nfrag_purged);
} else {
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 402e765d2..9c52aa282 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -130,6 +130,7 @@ struct dpif_ipf_proto_status {
uint64_t nfrag_expired_sent;
uint64_t nfrag_too_small;
uint64_t nfrag_overlap;
+ uint64_t nfrag_duplicate;
uint64_t nfrag_purged;
unsigned int min_frag_size;
bool enabled;
diff --git a/lib/ipf.c b/lib/ipf.c
index d836b8824..9adbd1fee 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -83,6 +83,7 @@ enum ipf_counter_type {
IPF_NFRAGS_EXPIRED,
IPF_NFRAGS_TOO_SMALL,
IPF_NFRAGS_OVERLAP,
+ IPF_NFRAGS_DUPLICATE,
IPF_NFRAGS_PURGED,
IPF_NFRAGS_NUM_CNTS,
};
@@ -806,26 +807,48 @@ ipf_list_key_lookup(struct ipf *ipf, const struct
ipf_list_key *key,
return NULL;
}
+/* Returns true if the new fragment is an exact duplicate of an existing
+ * fragment, i.e. it covers the same byte range. Per RFC 8200, an exact
+ * duplicate may be dropped on its own while keeping the rest of the
+ * fragment list for later reassembly. */
static bool
ipf_is_frag_duped(const struct ipf_frag *frag_list, int last_inuse_idx,
size_t start_data_byte, size_t end_data_byte)
/* OVS_REQUIRES(ipf_lock) */
{
for (int i = 0; i <= last_inuse_idx; i++) {
- if ((start_data_byte >= frag_list[i].start_data_byte &&
- start_data_byte <= frag_list[i].end_data_byte) ||
- (end_data_byte >= frag_list[i].start_data_byte &&
- end_data_byte <= frag_list[i].end_data_byte)) {
+ if (start_data_byte == frag_list[i].start_data_byte &&
+ end_data_byte == frag_list[i].end_data_byte) {
return true;
}
}
+
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. */
+/* Returns true if the new fragment overlaps any existing fragment without
+ * being an exact duplicate. Uses closed-interval overlap:
+ * start_a <= end_b && end_a >= start_b. */
+static bool
+ipf_is_frag_overlap(const struct ipf_frag *frag_list, int last_inuse_idx,
+ size_t start_data_byte, size_t end_data_byte)
+ /* OVS_REQUIRES(ipf_lock) */
+{
+ for (int i = 0; i <= last_inuse_idx; i++) {
+ if (start_data_byte <= frag_list[i].end_data_byte &&
+ end_data_byte >= frag_list[i].start_data_byte &&
+ !(start_data_byte == frag_list[i].start_data_byte &&
+ end_data_byte == frag_list[i].end_data_byte)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+/* Adds a fragment to a list of fragments. An exact duplicate fragment is
+ * dropped on its own, keeping the rest of the list for later reassembly
+ * (RFC 8200). An overlapping (but not duplicate) fragment is marked CT
+ * invalid, to avoid the work conntrack would otherwise do. */
static bool
ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
struct dp_packet *pkt, uint16_t start_data_byte,
@@ -835,9 +858,11 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
*ipf_list,
{
bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list,
ipf_list->last_inuse_idx, start_data_byte, end_data_byte);
+ bool overlap_frag = ipf_is_frag_overlap(ipf_list->frag_list,
+ ipf_list->last_inuse_idx, start_data_byte, end_data_byte);
int last_inuse_idx = ipf_list->last_inuse_idx;
- if (!duped_frag) {
+ if (!duped_frag && !overlap_frag) {
if (last_inuse_idx < ipf_list->size - 1) {
struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
frag->pkt = pkt;
@@ -850,6 +875,11 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list
*ipf_list,
} else {
OVS_NOT_REACHED();
}
+ } else if (duped_frag) {
+ /* RFC 8200: an exact duplicate may be dropped while keeping the rest
+ * of the fragment list for later reassembly. */
+ ipf_count(ipf, v6, IPF_NFRAGS_DUPLICATE);
+ dp_packet_delete(pkt);
} else {
ipf_count(ipf, v6, IPF_NFRAGS_OVERLAP);
pkt->md.ct_state = CS_INVALID;
@@ -1441,6 +1471,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
*ipf_status)
&ipf_status->v4.nfrag_too_small);
atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_OVERLAP],
&ipf_status->v4.nfrag_overlap);
+ atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_DUPLICATE],
+ &ipf_status->v4.nfrag_duplicate);
atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_PURGED],
&ipf_status->v4.nfrag_purged);
@@ -1457,6 +1489,8 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
*ipf_status)
&ipf_status->v6.nfrag_too_small);
atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_OVERLAP],
&ipf_status->v6.nfrag_overlap);
+ atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_DUPLICATE],
+ &ipf_status->v6.nfrag_duplicate);
atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_PURGED],
&ipf_status->v6.nfrag_purged);
return 0;
diff --git a/lib/ipf.h b/lib/ipf.h
index 7324a1704..238d288c1 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -28,6 +28,7 @@ struct ipf_proto_status {
uint64_t nfrag_expired_sent;
uint64_t nfrag_too_small;
uint64_t nfrag_overlap;
+ uint64_t nfrag_duplicate;
uint64_t nfrag_purged;
unsigned int min_frag_size;
bool enabled;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 60cead482..590059218 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5563,8 +5563,13 @@
recirc_id(<recirc>),in_port(90),packet_type(ns=0,id=0),eth(src=00:26:b9:8c:b0:f9
dnl Purge second frag
ovs-appctl time/warp 33000
-dnl Make sure all four packets are counted properly in the coverage.
-AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"], [0], [2
+dnl Make sure expiry and purge counters reflect the packets sent. The
+dnl duplicate second fragment is dropped on its own, so its list still expires.
+AT_CHECK([ovs-appctl coverage/show \
+| grep -c "ipf_stuck_frag_list_expired.*total: 1"], [0], [1
+])
+AT_CHECK([ovs-appctl coverage/show \
+| grep -c "ipf_stuck_frag_list_purged.*total: 2"], [0], [1
])
zero1208=$(printf '%0*d' 2416 0)
@@ -5669,6 +5674,60 @@ 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 an exact duplicate: per RFC 8200 it is dropped on its own
+dnl while the first fragment is kept for later reassembly, rather than being
+dnl marked CT 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 'num frag:|v4 frags accepted:|v4 frags duplicate:'], [], [dnl
+ num frag: 1
+ v4 frags accepted: 1
+ v4 frags duplicate: 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
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index f0d9121e3..e78c45979 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -179,6 +179,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
v4 frags expired: 0
v4 frags too small: 0
v4 frags overlapped: 0
+ v4 frags duplicate: 0
v4 frags purged: 0
min v6 frag size: 1280
v6 frags accepted: 0
@@ -186,6 +187,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
v6 frags expired: 0
v6 frags too small: 0
v6 frags overlapped: 0
+ v6 frags duplicate: 0
v6 frags purged: 0
])
])
@@ -209,6 +211,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v4 frags expired: 0
v4 frags too small: 0
v4 frags overlapped: 0
+ v4 frags duplicate: 0
v4 frags purged: 0
min v6 frag size: 1280
v6 frags accepted: 0
@@ -216,6 +219,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v6 frags expired: 0
v6 frags too small: 0
v6 frags overlapped: 0
+ v6 frags duplicate: 0
v6 frags purged: 0
Fragment Lists:
@@ -242,6 +246,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v4 frags expired: 0
v4 frags too small: 0
v4 frags overlapped: 0
+ v4 frags duplicate: 0
v4 frags purged: 0
min v6 frag size: 1280
v6 frags accepted: 30
@@ -249,6 +254,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
v6 frags expired: 0
v6 frags too small: 0
v6 frags overlapped: 0
+ v6 frags duplicate: 0
v6 frags purged: 0
Fragment Lists:
@@ -282,6 +288,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
FORMAT_FRAG_LIST()], [], [dnl
v4 frags expired: 0
v4 frags too small: 0
v4 frags overlapped: 0
+ v4 frags duplicate: 0
v4 frags purged: 0
min v6 frag size: 1280
v6 frags accepted: 0
@@ -289,6 +296,7 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m |
FORMAT_FRAG_LIST()], [], [dnl
v6 frags expired: 0
v6 frags too small: 0
v6 frags overlapped: 0
+ v6 frags duplicate: 0
v6 frags purged: 0
Fragment Lists:
--
2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
