On 10/21/26 2:41 PM, Eli Britstein wrote:
> IP reassembly must not accept data past an MF=0 fragment. Track
> last fragments, reject ranges beyond them on insert, and require the
> sorted rightmost fragment to be MF=0 before completing a list.
>
> An exact duplicate that carries the last-fragment (MF=0) flag records that
> flag on the stored copy and re-runs the state transition, matching the
> Linux kernel, so a datagram whose only MF=0 fragment arrives as a duplicate
> can still complete.
>
> The accompanying tests are added to tests/ofproto-dpif.at (using
> netdev-dummy/receive with variable-built packets and precomputed IPv4
> header checksums) rather than the system-traffic suite.
>
> Assisted-by: composer-2.5-fast, Cursor
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Eli Britstein <[email protected]>
> ---
Some more comments from an LLM review that seem reasonable:
This patch tracks MF=0 fragments in ipf, drops fragments whose data
lies beyond a known last fragment, and lets an exact MF=0 duplicate
mark the stored copy as last.
> + bool beyond_last = ipf_is_beyond_last_frag(ipf_list->frag_list,
> + ipf_list->last_inuse_idx, start_data_byte, end_data_byte, lf);
[...]
> + } else if (duped_frag) {
[...]
> + if (lf) {
[...]
> + if (start_data_byte == frag_list[i].start_data_byte &&
> + end_data_byte == frag_list[i].end_data_byte &&
> + !frag_list[i].last_frag) {
> + frag_list[i].last_frag = true;
> + *rp = ipf_list_state_transition(ipf, ipf_list, ff, lf,
> v6);
Can an MF=0 duplicate mark a middle fragment as last here?
The duped_frag branch is taken without looking at beyond_last. Say
fragments 0..399, 400..799 and 800..1199 are queued, all with MF=1,
and a copy of 400..799 arrives with MF=0. The stored 400..799 gets
last_frag set even though 800..1199 lies past it.
After that, ipf_list_complete() always fails on this check:
> + if (ipf_list->frag_list[i - 1].last_frag) {
> + return false;
> + }
The real last fragment, 1200..1599, is then dropped as beyond last.
The list stays queued until it expires, while the parent commit
completes the same sequence.
The same thing happens when 400..799 and 800..1199 (MF=0) are queued,
an MF=0 copy of 400..799 arrives, and then 0..399 arrives.
Linux discards an MF=0 fragment that ends before already queued data,
so does the "matching the Linux kernel" statement hold for this path?
---
> + frag_list[i].last_frag = true;
> + *rp = ipf_list_state_transition(ipf, ipf_list, ff, lf,
> v6);
> + break;
[...]
> + dp_packet_delete(pkt);
What gets forwarded when the MF=0 duplicate completes the list?
The stored frag_list[i].pkt was queued with MF=1. Only its last_frag
metadata changes, and the incoming packet that actually carries MF=0
is freed.
ipf_post_execute_reass_pkts() copies conntrack metadata and addresses
into the stored fragments, but not fragment flags.
ipf_send_frags_in_list() then sends frag_list[].pkt as is.
With 0..399 and 400..799 queued with MF=1 and an MF=0 copy of 400..799
arriving, both forwarded fragments still carry MF=1. Can the receiver
reassemble that?
Would something like this address both concerns?
} else if (duped_frag) {
/* RFC 8200: an exact duplicate may be dropped while keeping the rest
- * of the fragment list for later reassembly. If it is the last
- * fragment and the stored copy was not marked last, record it so the
- * datagram can complete. */
+ * of the fragment list for later reassembly. If it is a consistent
+ * last fragment and the stored copy was not marked last, keep the
+ * MF=0 copy so the forwarded fragments end with MF=0. */
ipf_count(ipf, v6, IPF_NFRAGS_DUPLICATE);
- if (lf) {
+ if (lf && !beyond_last) {
struct ipf_frag *frag_list = ipf_list->frag_list;
for (int i = 0; i <= ipf_list->last_inuse_idx; i++) {
if (start_data_byte == frag_list[i].start_data_byte &&
end_data_byte == frag_list[i].end_data_byte &&
!frag_list[i].last_frag) {
+ struct dp_packet *stored = frag_list[i].pkt;
+
+ frag_list[i].pkt = pkt;
frag_list[i].last_frag = true;
+ pkt = stored;
*rp = ipf_list_state_transition(ipf, ipf_list, ff, lf, v6);
break;
}
With that change, the first sequence above completes once the real last
fragment arrives. In the second case, the MF=0 copy is the one that
gets forwarded. Two ofproto-dpif tests that check the exact ovs-pcap
output pass with the change and fail without it, if they would help:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ac009408a..1f79bae0c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6098,6 +6098,99 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
OVS_VSWITCHD_STOP
AT_CLEANUP
+dnl IPF_DUP_FRAG(FLAGS_OFFSET, CSUM)
+dnl
+dnl Expands to an IPv4 UDP fragment 10.1.1.1 -> 10.1.1.2, ID 0x0021, total
+dnl length 420 (400 zero data bytes), with the given 16-bit flags/fragment
+dnl offset field and header checksum, both in hex.
+m4_define([IPF_DUP_FRAG],
+ [50540000000a5054000000090800450001a40021$1[]4011$2[]0a0101010a010102[]dnl
+m4_for([i], [1], [400], [1], [00])])
+
+AT_SETUP([ofproto-dpif - fragment handling - last duplicate of middle
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])
+
+AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 400], [], [dnl
+setting minimum fragment size successful
+])
+
+dnl Bytes 0..399, MF set.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([2000], [4324])"])
+dnl Bytes 400..799, MF set.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([2032], [42f2])"])
+dnl Bytes 800..1199, MF set.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([2064], [42c0])"])
+dnl Bytes 400..799, MF clear: duplicate of the middle fragment. Queued data
+dnl lies past it, so it is dropped without marking the stored copy as last.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([0032], [62f2])"])
+dnl Bytes 1200..1599, MF clear: the real last fragment, completes the datagram.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([0096], [628e])"])
+
+AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
+| grep -E 'num frag:|v4 frags accepted:|v4 frags completed:|v4 frags
duplicate:|v4 frags beyond last:'], [], [dnl
+ num frag: 0
+ v4 frags accepted: 4
+ v4 frags completed: 4
+ v4 frags duplicate: 1
+ v4 frags beyond last: 0
+])
+
+dnl The four original fragments are forwarded in offset order.
+AT_CHECK([ovs-pcap p1-tx.pcap], [0], [dnl
+IPF_DUP_FRAG([2000], [4324])
+IPF_DUP_FRAG([2032], [42f2])
+IPF_DUP_FRAG([2064], [42c0])
+IPF_DUP_FRAG([0096], [628e])
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - fragment handling - last fragment arrives as
duplicate])
+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])
+
+AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 400], [], [dnl
+setting minimum fragment size successful
+])
+
+dnl Bytes 0..399, MF set.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([2000], [4324])"])
+dnl Bytes 400..799, MF set.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([2032], [42f2])"])
+dnl Bytes 400..799, MF clear: duplicate of the second fragment that completes
+dnl the datagram. It replaces the stored MF=1 copy.
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "IPF_DUP_FRAG([0032], [62f2])"])
+
+AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
+| grep -E 'num frag:|v4 frags accepted:|v4 frags completed:|v4 frags
duplicate:'], [], [dnl
+ num frag: 0
+ v4 frags accepted: 2
+ v4 frags completed: 2
+ v4 frags duplicate: 1
+])
+
+dnl The first fragment and the MF=0 copy of the second are forwarded, so the
+dnl receiver gets a last fragment.
+AT_CHECK([ovs-pcap p1-tx.pcap], [0], [dnl
+IPF_DUP_FRAG([2000], [4324])
+IPF_DUP_FRAG([0032], [62f2])
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
OVS_VSWITCHD_START
add_of_ports br0 1 90
---
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
