On 10/4/26 12:14 PM, Eli Britstein via dev 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.
>
> The accompanying test is 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]>
> ---
Continuation of the LLM review:
This patch rejects fragments that would place data past an MF=0 fragment
and requires the rightmost fragment to be the last fragment before
completing reassembly.
> + if (lf && frag_list[i].end_data_byte > end_data_byte) {
> + return true;
> + }
Could the test also cover this condition? The added test stores the MF=0
fragment first and then sends data beyond it, which only exercises the
first check in ipf_is_beyond_last_frag().
For this one, a test could queue [800..1199] with MF=1 and then send
[400..799] with MF=0, checking the beyond-last counter.
> + num frag: 1
> + v4 frags accepted: 1
> + v4 frags completed: 0
> + v4 frags beyond last: 1
Since the stored last fragment is expected to stay queued, could the
test then send the missing [0..399] fragment and check that reassembly
completes? That would confirm the retained list is still usable.
Could there also be an IPv6 case covering the v6 beyond-last counter?
> + } else {
> + ipf_count(ipf, v6, IPF_NFRAGS_BEYOND_LAST);
> + dp_packet_delete(pkt);
> + return true;
> + }
The comment above ipf_process_frag() still lists only the duplicate and
overlap outcomes. Should it mention the beyond-last case too?
Also, the "return true;" here is redundant with the one after the
if/else chain.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
