Messages in this thread | | | Subject | Re: [PATCH net] net: ipv4: do not handle duplicate fragments as overlapping | From | Eric Dumazet <> | Date | Wed, 12 Dec 2018 22:20:42 -0800 |
| |
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 !
| |