Re: [PATCH net] net: ipv4: do not handle duplicate fragments as overlapping

[Eric Dumazet <eric.dumazet@gmail.com>]

On 12/12/2018 06:28 PM, Michal Kubecek wrote:
> Since commit 7969e5c40dfd ("ip: discard IPv4 datagrams with overlapping
> segments.") IPv4 reassembly code drops the whole queue whenever an
> overlapping fragment is received. However, the test is written in a way
> which detects duplicate fragments as overlapping so that in environments
> with many duplicate packets, fragmented packets may be undeliverable.
> 
> Add an extra test and for (potentially) duplicate fragment, only drop the
> new fragment rather than the whole queue. Only starting offset and length
> are checked, not the contents of the fragments as that would be too
> expensive.  Check for duplicity with last (tail) fragment first as in real
> life scenarios this should be the most frequent case and we would have to
> iterate through the whole "run" otherwise.
> 
> Fixes: 7969e5c40dfd ("ip: discard IPv4 datagrams with overlapping segments.")
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
>  net/ipv4/ip_fragment.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index aa0b22697998..f09e3683b209 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -436,6 +436,10 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
>  			ip4_frag_append_to_last_run(&qp->q, skb);
>  		else
>  			ip4_frag_create_run(&qp->q, skb);
> +	} else if (offset == prev_tail->ip_defrag_offset &&
> +		   skb->len == prev_tail->len) {
> +		/* potential duplicate of last fragment */
> +		goto err;

What value is in @err variable at this point ?

Are you sure callers expect to receive -EINVAL ?



>  	} else {
>  		/* Binary search. Note that skb can become the first fragment,
>  		 * but not the last (covered above).
> @@ -449,8 +453,16 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
>  			else if (offset >= skb1->ip_defrag_offset +
>  						FRAG_CB(skb1)->frag_run_len)
>  				rbn = &parent->rb_right;
> -			else /* Found an overlap with skb1. */
> +			else {
> +				/* check for potential duplicate */
> +				while (skb1 && skb1->ip_defrag_offset < offset)
> +					skb1 = FRAG_CB(skb1)->next_frag;
> +				if (skb1 && offset == skb1->ip_defrag_offset &&
> +				    skb->len == skb1->len)
> +					goto err;

Maybe we should not care, if the node in the rbtree contains the range of this
incoming fragment, do not worry about finding if it is overlap or not ?

I am nervous about adding back a linear scan.

> +				/* Found an overlap */
>  				goto overlap;
> +			}
>  		} while (*rbn);
>  		/* Here we have parent properly set, and rbn pointing to
>  		 * one of its NULL left/right children. Insert skb.
> 

Thanks !