This is an AI generated review of your patch. A human has looked at the
results and deemed any concerns as plausible.
Verdict reasoning summary: the change swaps "mark CT invalid and hand
the fragment back to the conntrack batch" for "free the fragment",
keeping the same return convention, so no leak or double free is
introduced.
Hunk 1 (lib/ipf.c, duplicate/overlap branch)
> } else {
> ipf_count(ipf, v6, IPF_NFRAGS_OVERLAP);
> - pkt->md.ct_state = CS_INVALID;
> - return false;
> + dp_packet_delete(pkt);
> + return true;
Ownership is correct. `ipf_handle_frag()`/`ipf_process_frag()` use "true
= packet consumed, false = caller re-adds", and the only caller
(`ipf_extract_frags_from_batch`, lib/ipf.c:1038-1062) does exactly that
with `dp_packet_batch_add(pb, pkt)` on false. The iteration uses
`DP_PACKET_BATCH_REFILL_FOR_EACH`, whose contract in
lib/dp-packet.h:945-952 is "caller owns the packets that are not
refilled", so deleting here rather than refilling is the intended drop
path and matches the existing drops a few lines up (the
`ipf_v4_key_extract`/`ipf_v6_key_extract` failure paths in
`ipf_handle_frag`, which already do `dp_packet_delete(pkt); return
true;`). No double free: the refill iterator truncates the batch to the
refilled prefix, so the freed pointer is never visited again.
Accounting is also consistent: `atomic_count_inc(&ipf->nfrag)` happens
only on the accept branch, so a rejected duplicate must not decrement
it, and it does not. The `IPF_NFRAGS_OVERLAP` counter is preserved,
which keeps `dpctl/ipf-get-status` output meaningful (and the system
testsuite expectations of `overlapped: 0` are unaffected). Freeing under
`ipf->ipf_lock` is slightly heavier work inside the lock but is already
the established pattern in this file.
One behavioral consequence worth stating rather than hiding: overlapping
fragments no longer reach conntrack at all, so they no longer produce a
`+inv+trk` result. Deployments whose policy relies on matching/dropping
`ct_state=+inv+trk`, or on conntrack stats/ct events to observe such
traffic, lose that visibility; the drop is silent apart from the IPF
counter. That is what the commit message intends and matches the kernel,
so it is a note, not a defect. Note also an asymmetry that this commit
does not address: once a list reaches
`IPF_LIST_STATE_COMPLETED`/`REASS_FAIL`, `ipf_handle_frag()` returns
false early and such fragments still go to conntrack instead of being
dropped — pre-existing structure, unchanged here.
Hunk 2 (tests/ofproto-dpif.at, new test)
The packet construction checks out. I verified the IPv4 header checksum
by summing the words: `4500 01a4 0020 0032 4011` plus `0a01 0101 0a01
0102` = `0x9d0c`, and `0x9d0c + 62f3 = 0xffff`, so the header is valid —
which matters, because `ipf_is_valid_v4_frag()` recomputes the checksum
and would otherwise take the `invalid_pkt` path (which sets `CS_INVALID`
and bypasses the overlap counter entirely). The MF=0 last-fragment
choice also correctly sidesteps the minimum-fragment-size check, since
that check in lib/ipf.c:672-677 is guarded by `!lf`, so the test does
not need `dpctl/ipf-set-min-frag`.
Minor concern: the zero-output assertion
> AT_CHECK([test 0 = `ovs-ofctl parse-pcap p1-tx.pcap | wc -l`])
is only meaningful if `p1-tx.pcap` actually exists. The comparable
existing test (tests/ofproto-dpif.at:13935-13943) first uses
`OVS_WAIT_UNTIL` on a positive pcap event before asserting a zero count
elsewhere. Here the preceding `ipf-get-status` check does act as a
de-facto sync point (the overlap counter is bumped before any forwarding
decision), so I do not expect flakiness, but if the pcap file were never
created the assertion would either fail on unexpected stderr or pass
vacuously — a weak rather than wrong check. Also, there is no IPv6
duplicate case and no case exercising an overlap arriving after
reassembly completed; coverage of the drop path is v4-only.
Verdict: WARN
```json
{
"findings": [
{
"type": "Logic",
"title": "Overlapping fragments are now dropped silently, bypassing
conntrack and its observability",
"description": "ipf_process_frag() no longer sets pkt->md.ct_state =
CS_INVALID and returns the fragment to the batch; it frees the fragment
instead. The packet never reaches conntrack, so no +inv+trk state is produced
and no conntrack statistics or ct events are generated for it.",
"reasoning": "Verified the caller contract:
ipf_extract_frags_from_batch() (lib/ipf.c:1038-1062) re-adds the packet only
when ipf_handle_frag() returns false, and DP_PACKET_BATCH_REFILL_FOR_EACH
documents that non-refilled packets belong to the caller
(lib/dp-packet.h:945-952), so freeing is correct and leak-free. The consequence
is purely behavioral: policies or monitoring keyed on ct_state=+inv+trk for
overlapping fragments lose their signal; only the IPF_NFRAGS_OVERLAP counter
remains. This matches the stated intent (aligning with the kernel) but is an
externally visible change worth documenting.",
"severity": "low",
"severity_explanation": "No memory or correctness breakage; triggering
path is any overlapping/duplicate IPv4 or IPv6 fragment processed through the
userspace datapath conntrack path. Impact limited to loss of visibility and a
change for flows that deliberately forward invalid packets.",
"locations": [
{
"file": "lib/ipf.c",
"function_or_symbol": "ipf_process_frag",
"line_range": "851-856",
"why_this_location_matters": "This is where the duplicate fragment
stops being handed back to conntrack and is freed instead, so all downstream
ct_state-dependent behavior changes."
}
],
"preexisting": false
},
{
"type": "Logic",
"title": "New test's zero-output check can pass vacuously if p1-tx.pcap
is absent",
"description": "The assertion 'test 0 = `ovs-ofctl parse-pcap p1-tx.pcap
| wc -l`' does not first establish that the pcap file exists or that any packet
was ever written to it, unlike the similar existing test at
tests/ofproto-dpif.at:13935-13943 which gates on OVS_WAIT_UNTIL for a positive
event.",
"reasoning": "Read both tests and the add_of_ports --pcap helper
(tests/ofproto-macros.at:341-380). The preceding dpctl/ipf-get-status check
does synchronize with packet processing because the overlap counter is
incremented before any forwarding decision, so I expect no flakiness; but the
assertion itself carries no positive guarantee that output would have been
recorded had the fragment been forwarded.",
"severity": "low",
"severity_explanation": "Test-strength issue only. If forwarding behavior
regressed in a way that also prevented pcap creation, or if the file naming
changed, the test could pass without exercising the intended property.",
"locations": [
{
"file": "tests/ofproto-dpif.at",
"function_or_symbol": "AT_SETUP([ofproto-dpif - fragment handling -
drop duplicate fragment])",
"line_range": "5672-5723",
"why_this_location_matters": "This is the only assertion verifying
that the dropped duplicate is never transmitted."
}
],
"preexisting": false
},
{
"type": "Logic",
"title": "Duplicate fragments after reassembly completion still bypass
the new drop path",
"description": "ipf_handle_frag() returns false early when the matching
list is in IPF_LIST_STATE_COMPLETED or IPF_LIST_STATE_REASS_FAIL, so a
duplicate arriving at that point is returned to conntrack rather than dropped,
inconsistent with the new overlap handling.",
"reasoning": "Read ipf_handle_frag() (lib/ipf.c:957-1035): the early
bail-out on those two states precedes any call to ipf_process_frag(), so it is
unreachable from the new drop logic. This structure predates the commit and is
not made worse by it, but the commit's stated goal of discarding duplicates in
IPF rather than conntrack is only partially achieved.",
"severity": "low",
"severity_explanation": "Narrow window: requires a duplicate fragment
arriving after the datagram already reassembled or failed. Behavior is
unchanged from before the commit, so this is an incomplete-coverage note rather
than a regression.",
"locations": [
{
"file": "lib/ipf.c",
"function_or_symbol": "ipf_handle_frag",
"line_range": "1016-1020",
"why_this_location_matters": "Early return for completed/failed lists
skips ipf_process_frag() and therefore the new drop."
}
],
"preexisting": true
}
]
}
```
[note: tool call budget exhausted]
---
findings: 3 worst severity: low tokens: 965456 in / 23877 out
Eli Britstein <[email protected]> writes:
> When IPF detects an overlapping or duplicate fragment, drop it instead
> of marking its CT state invalid and returning it to the conntrack batch.
> This aligns with the Linux kernel, which discards such fragments during
> IP reassembly rather than forwarding them separately.
>
> Assisted-by: composer-2.5-fast, Cursor
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> lib/ipf.c | 8 +++----
> tests/ofproto-dpif.at | 52 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index d836b8824..185d6432e 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -823,8 +823,8 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int
> last_inuse_idx,
> }
>
> /* 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
> + * duplicate. If the fragment is a duplicate, the fragment is dropped
> + * to avoid the work that conntrack would do to mark the fragment
> * as invalid, which it will in all cases. */
> static bool
> ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
> @@ -852,8 +852,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);
> + return true;
> }
> return true;
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index ee6ac873d..5308b3008 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
