This is an AI generated review of your patch. A human has looked at the
results and deemed any concerns as plausible.
Changed files: lib/ipf.c (overlap predicate), tests/ofproto-dpif.at (new
test). Purpose: the previous duplicate/overlap test only caught a new
fragment whose start or end landed inside an existing fragment's range,
so a new fragment that strictly encloses an older one (start < old.start
and end > old.end) passed. The new closed-interval test `start_new <=
end_old && end_new >= start_old` is the standard interval-intersection
predicate and covers containment plus both partial-overlap cases.
Correctness of the predicate
> 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].end_data_byte &&
> + end_data_byte >= frag_list[i].start_data_byte) {
Verified against the invariants of the callers:
- Ranges are inclusive on both ends. ipf_v4_key_extract
(lib/ipf.c:705-706) and ipf_v6_key_extract (lib/ipf.c:806-807) compute
`end = start + len - 1`, so the closed-interval comparison is the
right form; adjacent fragments (`start_new == end_old + 1`) are
correctly *not* flagged, which matters because ipf_list_complete
(lib/ipf.c:397-401) requires exactly that contiguity.
- No signedness/overflow exposure: parameters are `size_t`, stored
fields are `uint16_t` and the producers already reject ranges that
would wrap past UINT16_MAX (lib/ipf.c:697-703, 799-804), so the
int-promoted uint16_t operands can never be negative when converted to
the unsigned comparison type.
- Only caller is ipf_process_frag (lib/ipf.c:903-906); on overlap it
bumps IPF_NFRAGS_OVERLAP and deletes the packet, which is
unchanged. `ipf_is_beyond_last_frag` is still evaluated independently
and the duped branch takes precedence in counting — no double-count
and no counter regression.
- Tightening policy: rejecting any overlap matches RFC 5722 discard
behavior and matches what conntrack would do downstream anyway (the
stated rationale in the comment above ipf_process_frag). I grepped for
tests expecting overlapping fragments to be accepted; only the
exact-duplicate test (tests/ofproto-dpif.at:5713-5716) expects a
nonzero overlap counter, and exact duplicates were already caught by
the old predicate, so no existing expectation
changes. system-traffic-derived suites all expect `frags overlapped:
0`.
Test hunk
I checked the hand-written packet bytes rather than trusting the
wireshark-style comments:
- Both IP header checksums are correct. Using the sibling last-fragment
test (tests/ofproto-dpif.at:5704, same addresses/TTL/proto/ID/tot_len,
frag_off 0x0032, csum 0x62f3) as the base one's-complement sum,
flipping the MF bit adds 0x2000 and offset 0x2032 adds 0x2000 (net
zero), giving 0x42f3 for packet1; for packet2 frag_off 0x2000 with
tot_len 0x04c4 gives 0x4005. A bad checksum would have made
ipf_is_valid_v4_frag mark the packet CS_INVALID and the test would
pass for the wrong reason, so this mattered to check.
- `printf '%0*d' 800 0` / `2400 0` produce 400 and 1200 payload bytes
respectively, consistent with tot_len 420 and 1220
(ipf_is_valid_v4_frag requires `ip_tot_len == l3_size`).
- packet1 is a middle fragment (MF set) at offset 400 with l3 size 420,
so it clears the minimum-size gate; packet2 covers bytes 0..1199 and
fully contains 400..799 — exactly the case the old code
missed. Expected counters (`num frag: 1`, accepted 1, completed 0,
overlapped 1) follow from that.
One documentation nit in the test comment:
> dnl The minimum fragment size is clamped to 400 bytes, so both fragments must
> dnl be at least that large to be admitted for reassembly.
Two inaccuracies here (lib/ipf.c:1479 and 675): values below 400 are
rejected by `ipf-set-min-frag` with an error, not clamped; and the size
gate applies only to non-last fragments (`!lf`) and is compared against
`dp_packet_l3_size()` (header included), not the payload. Harmless to
the assertions, but the wording invites a future edit that assumes last
fragments are also size-gated.
Also worth noting (pre-existing, not worsened): the overlap scan is O(n)
per fragment over an unsorted array, so admission is quadratic in
fragment count; the commit keeps that shape.
Verdict: PASS
```json
{
"findings": [
{
"type": "Documentation",
"title": "Test comment misdescribes the minimum fragment size rule",
"description": "The new test's comment states the minimum fragment size
is 'clamped to 400 bytes' and that 'both fragments must be at least that
large'. In fact ipf_set_min_frag() rejects values below IPF_V4_FRAG_SIZE_LBOUND
with an error rather than clamping, and the size gate in ipf_is_valid_v4_frag()
applies only to non-last fragments (condition '!lf') and is compared against
dp_packet_l3_size() (IP header included), not payload length.",
"reasoning": "Read lib/ipf.c:1475-1494 (lower-bound rejection, no
clamping) and lib/ipf.c:672-678 ('!lf && dp_packet_l3_size(pkt) <
min_v4_frag_size_'). The assertions themselves are unaffected because packet1
is a middle fragment with l3 size 420 >= 400, but the comment can mislead a
future maintainer into thinking last fragments are size-gated or that an
out-of-range value silently succeeds.",
"severity": "low",
"severity_explanation": "No functional impact; only risks incorrect
assumptions when the test is modified or copied.",
"locations": [
{
"file": "tests/ofproto-dpif.at",
"function_or_symbol": "ofproto-dpif - fragment handling - reject
overlapped fragment",
"line_range": "5780-5782",
"why_this_location_matters": "Comment asserts clamping and a size
requirement on both fragments, which does not match the implementation."
},
{
"file": "lib/ipf.c",
"function_or_symbol": "ipf_set_min_frag",
"line_range": "1475-1494",
"why_this_location_matters": "Shows values below the bound return an
error instead of being clamped."
},
{
"file": "lib/ipf.c",
"function_or_symbol": "ipf_is_valid_v4_frag",
"line_range": "672-678",
"why_this_location_matters": "Shows the gate is skipped for last
fragments and measures L3 size, not payload."
}
],
"preexisting": false
}
]
}
```
---
findings: 1 worst severity: low tokens: 514973 in / 38910 out
Eli Britstein <[email protected]> writes:
> The previous overlap check only tested whether a new fragment's
> endpoints fell inside an existing range, missing the case where the
> new fragment completely contains an older one. Use closed-interval
> overlap so conflicting fragments are rejected.
>
> Assisted-by: composer-2.5-fast, Cursor
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Acked-by: Mike Pattrick <[email protected]>
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> lib/ipf.c | 8 ++---
> tests/ofproto-dpif.at | 73 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 72ebd238a..2b016e60b 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -843,16 +843,16 @@ ipf_list_key_lookup(struct ipf *ipf, const struct
> ipf_list_key *key,
> return NULL;
> }
>
> +/* Returns true if the new fragment overlaps any existing fragment. Uses
> + * closed-interval overlap: start_a <= end_b && end_a >= start_b. */
> 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].end_data_byte &&
> + end_data_byte >= frag_list[i].start_data_byte) {
> return true;
> }
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 0e84dc308..0ff4c50f8 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5769,6 +5769,79 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - fragment handling - reject overlapped fragment])
> +OVS_VSWITCHD_START
> +add_of_ports 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 The minimum fragment size is clamped to 400 bytes, so both fragments must
> +dnl be at least that large to be admitted for reassembly.
> +AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 400], [], [dnl
> +setting minimum fragment size successful
> +])
> +
> +dnl First admit a fragment carrying bytes 400..799, then reject a new
> fragment
> +dnl covering bytes 0..1199 that fully contains the previously admitted range.
> +dnl
> +dnl Packet 1 (admitted). Middle fragment carrying bytes 400..799 (MF set).
> +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 001. .... = Flags: 0x1 (More fragments)
> +dnl ...0 0000 0011 0010 = Fragment Offset: 400
> +dnl Time to Live: 64
> +dnl Protocol: UDP (17)
> +dnl Header Checksum: 0x42f3
> +dnl Data (400 bytes)
> +eth="50 54 00 00 00 0a 50 54 00 00 00 09 08 00"
> +ip1="45 00 01 a4 00 20 20 32 40 11 42 f3"
> +addrs="0a 01 01 01 0a 01 01 02"
> +data1=$(printf '%0*d' 800 0)
> +packet1="${eth}${ip1}${addrs}${data1}"
> +
> +dnl Packet 2 (rejected as overlap). First fragment carrying bytes 0..1199
> +dnl (MF set), fully containing packet 1's byte range.
> +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: 1220
> +dnl Identification: 0x0020 (32)
> +dnl 001. .... = Flags: 0x1 (More fragments)
> +dnl ...0 0000 0000 0000 = Fragment Offset: 0
> +dnl Time to Live: 64
> +dnl Protocol: UDP (17)
> +dnl Header Checksum: 0x4005
> +dnl Data (1200 bytes)
> +ip2="45 00 04 c4 00 20 20 00 40 11 40 05"
> +data2=$(printf '%0*d' 2400 0)
> +packet2="${eth}${ip2}${addrs}${data2}"
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet1"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$packet2"])
> +
> +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m \
> +| grep -E 'num frag:|v4 frags accepted:|v4 frags completed:|v4 frags
> overlapped:'], [], [dnl
> + num frag: 1
> + v4 frags accepted: 1
> + v4 frags completed: 0
> + v4 frags overlapped: 1
> +])
> +
> +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
