On 10/4/26 12:14 PM, Eli Britstein via dev wrote:
> 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(-)
Hi, Eli, others. I ran this set through some LLM review and got the
following comments that seems reasonable. The commit itself is kind of
misleading, it sort of misstates the wording of the RFC and introduces
a questionable behavior that is reverted a few patches down the set.
We should probably do things right right away.
Best regards, Ilya Maximets.
The review:
This patch drops the entire queued datagram when an overlapping fragment
arrives.
> } 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);
> }
Could the exact-duplicate distinction from the later patch be introduced
before, or together with, this change?
At this point ipf_is_frag_duped() also matches identical fragments, so a
duplicate now deletes the previously accepted copy and the rest of the
datagram. The parent kept the queued fragments in that case. The later
patch restores that behavior, but the intermediate state discards
datagrams containing duplicate fragments.
> 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.
For exact duplicates the remaining list could still complete in the
parent, so is "unreassemblable" accurate for that case?
> + 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);
> + }
Could the previously accepted fragments discarded here be counted
somewhere? The incoming fragment increments IPF_NFRAGS_OVERLAP, but
nothing in ipf-get-status records the queued fragments deleted by this
helper.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
