RE: [PATCH 1/6] ip_frag: tolerate duplicate fragments

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> The reassembly code tracked only a running byte total and reserved slots
> for the first and last fragments, with no check for a fragment
> duplicating data already received. A single duplicate could destroy a
> recoverable datagram:
>  - a duplicate first or last fragment collided with the reserved slot and
>    sent the whole entry down the error path, freeing every collected
>    fragment;
>  - a duplicate intermediate fragment was appended to a new slot, inflating
>    frag_size past total_size so reassembly never completed.
> 
> RFC 791 reassembly tolerates duplicates: a fragment covering bytes
> already present carries no new information. Check for an exact duplicate
> (stored fragment with the same offset and length) and drop only that
> mbuf, before frag_size is updated, leaving the entry's accounting
> unchanged.
> 
> Overlapping fragments with differing bounds are a separate issue
> addressed in the next patch.
> 
> Fixes: cc8f4d020c0b ("examples/ip_reassembly: initial import")
> Cc: stable@dpdk.org

I am not sure it is a bug and needs to be propagated into the stable releases.
To me it is more like feature improvement.
BTW, as this and next patch does change the behavior and probably overall
performance numbers, - it probably worth to add a line in the release notes.
As another thought - it might be squashed with next patch in the series
(ip_frag: discard datagrams with overlapping fragments).
Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

> Reported-by: Samyak Jain <samyak.jain@amantyatech.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ip_frag/ip_frag_internal.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ip_frag/ip_frag_internal.c b/lib/ip_frag/ip_frag_internal.c
> index 382f42d0e1..9a03ef995a 100644
> --- a/lib/ip_frag/ip_frag_internal.c
> +++ b/lib/ip_frag/ip_frag_internal.c
> @@ -89,7 +89,23 @@ struct rte_mbuf *
>  ip_frag_process(struct ip_frag_pkt *fp, struct rte_ip_frag_death_row *dr,
>  	struct rte_mbuf *mb, uint16_t ofs, uint16_t len, uint16_t more_frags)
>  {
> -	uint32_t idx;
> +	uint32_t i, idx;
> +
> +	/*
> +	 * Discard an exact duplicate fragment. If a previously stored fragment
> +	 * already covers the same offset and length, this fragment carries no
> +	 * new data. Reassembly is tolerant of duplicates (RFC 791), so drop
> +	 * only this mbuf and keep the reassembly entry intact rather than
> +	 * treating it as an error. Fragments overlapping an existing one with
> +	 * different bounds are not handled here.
> +	 */
> +	for (i = 0; i != fp->last_idx; i++) {
> +		if (fp->frags[i].mb != NULL && fp->frags[i].ofs == ofs &&
> +				fp->frags[i].len == len) {
> +			IP_FRAG_MBUF2DR(dr, mb);
> +			return NULL;
> +		}
> +	}
> 
>  	fp->frag_size += len;
> 
> --
> 2.53.0