Eli Britstein via dev <[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.
I would consider rephrasing this slightly - the kernel actually spells
out why we should drop these (while it mentions ipv6 explicitly, ipv4
and v6 follow the same logic - from net/ipv4/inet_fragment.c):
/* RFC5722, Section 4, amended by Errata ID : 3089
* When reassembling an IPv6 datagram, if
* one or more its constituent fragments is determined to be an
* overlapping fragment, the entire datagram (and any constituent
* fragments) MUST be silently discarded.
*
* Duplicates, however, should be ignored (i.e. skb dropped, but the
* queue/fragments kept for later reassembly).
*/
This isn't really what this patch is doing, since we don't discard the
entire overlapping region. Instead we are just doing the silent
discard, and treating overlaps + dups the same.
At the very least, the commit message should be rephrased to match that
we are not complying with RFC 5722 / RFC 8200 (or we could update to
match those and drop the entire fragment chain):
When IPF detects overlap or duplicate, silently discard the overlap or
duplicate. This diverges slightly from a compliant RFC 5722-style
fragment handling which should silently drop the entire fragment chain
when an overlap is detected, but it matches closer to other RFC
compliant stacks by not forwarding along the fragment when conntrack
is involved in the forwarding chain.
Alternatively, you could also add code that will just drop the entire
chain when the fragment is overlapping.
> 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
It would be good to actually mention the RFC rather than just saying
we're trying to avoid work.
> * 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;
This return seems excessive, given the below to just 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
