[PATCH]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[PATCH]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Shan Wei]

No matter whether connection track is enabled, an end host should send 
an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
The reasons are following two points:  

1. RFC 792 says:
   >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
   >>>> >> > >   reassembly due to missing fragments within its time limit it
   >>>> >> > >   discards the datagram, and it may send a time exceeded message.
   >>>> >> > > 
   >>>> >> > >   If fragment zero is not available then no time exceeded need be
   >>>> >> > >   sent at all.
   >>>> >> > > 
   >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp

2. Patrick McHardy also agrees with this opinion. :-) 
   About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649

The patch fixed the problem like this:
When enabling connection track, fragments are received at PRE_ROUTING HOOK.
If they are failed to reassemble, ip_expire() will be called. 
Before sending an ICMP "Fragment Reassembly Timeout" message, 
the patch searches router table to get the destination entry only for host type.

The patch has been tested on both host type and route type.


Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com> 
---
 net/ipv4/ip_fragment.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 86964b3..bee6905 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -32,6 +32,8 @@
 #include <linux/netdevice.h>
 #include <linux/jhash.h>
 #include <linux/random.h>
+#include <net/route.h>
+#include <net/dst.h>
 #include <net/sock.h>
 #include <net/ip.h>
 #include <net/icmp.h>
@@ -205,13 +207,38 @@ static void ip_expire(unsigned long arg)
 	if ((qp->q.last_in & INET_FRAG_FIRST_IN) && qp->q.fragments != NULL) {
 		struct sk_buff *head = qp->q.fragments;
 
-		/* Send an ICMP "Fragment Reassembly Timeout" message. */
 		rcu_read_lock();
 		head->dev = dev_get_by_index_rcu(net, qp->iif);
-		if (head->dev)
-			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
-		rcu_read_unlock();
+		if (!head->dev)
+			goto out_rcu_unlock;
+
+		/*
+		 * Only search router table for the head fragment,
+		 * when defraging timeout at PRE_ROUTING HOOK.
+		 */
+		if (qp->user == IP_DEFRAG_CONNTRACK_IN && !skb_dst(head)) {
+			const struct iphdr *iph = ip_hdr(head);
+			int err = ip_route_input(head, iph->daddr, iph->saddr,
+						 iph->tos, head->dev);
+			if (unlikely(err))
+				goto out_rcu_unlock;
+
+			/*
+			 * Only an end host needs to send an ICMP
+			 * "Fragment Reassembly Timeout" message, per RFC792.
+			 */
+			if (skb_rtable(head)->rt_type != RTN_LOCAL) {
+				skb_dst_drop(head);
+				goto out_rcu_unlock;
+			}
+		}
+
+		/* Send an ICMP "Fragment Reassembly Timeout" message. */
+		icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
 	}
+
+out_rcu_unlock:
+	rcu_read_unlock();
 out:
 	spin_unlock(&qp->q.lock);
 	ipq_put(qp);
-- 
1.6.3.3

Re: [PATCH]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Patrick McHardy]

Shan Wei wrote:
> No matter whether connection track is enabled, an end host should send 
> an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
> The reasons are following two points:  
> 
> 1. RFC 792 says:
>    >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
>    >>>> >> > >   reassembly due to missing fragments within its time limit it
>    >>>> >> > >   discards the datagram, and it may send a time exceeded message.
>    >>>> >> > > 
>    >>>> >> > >   If fragment zero is not available then no time exceeded need be
>    >>>> >> > >   sent at all.
>    >>>> >> > > 
>    >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp
> 
> 2. Patrick McHardy also agrees with this opinion. :-) 
>    About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649
> 
> The patch fixed the problem like this:
> When enabling connection track, fragments are received at PRE_ROUTING HOOK.
> If they are failed to reassemble, ip_expire() will be called. 
> Before sending an ICMP "Fragment Reassembly Timeout" message, 
> the patch searches router table to get the destination entry only for host type.
> 
> The patch has been tested on both host type and route type.

Looks good. One comment below:

> @@ -205,13 +207,38 @@ static void ip_expire(unsigned long arg)
>  	if ((qp->q.last_in & INET_FRAG_FIRST_IN) && qp->q.fragments != NULL) {
>  		struct sk_buff *head = qp->q.fragments;
>  
> -		/* Send an ICMP "Fragment Reassembly Timeout" message. */
>  		rcu_read_lock();
>  		head->dev = dev_get_by_index_rcu(net, qp->iif);
> -		if (head->dev)
> -			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
> -		rcu_read_unlock();
> +		if (!head->dev)
> +			goto out_rcu_unlock;
> +
> +		/*
> +		 * Only search router table for the head fragment,
> +		 * when defraging timeout at PRE_ROUTING HOOK.
> +		 */
> +		if (qp->user == IP_DEFRAG_CONNTRACK_IN && !skb_dst(head)) {
> +			const struct iphdr *iph = ip_hdr(head);
> +			int err = ip_route_input(head, iph->daddr, iph->saddr,
> +						 iph->tos, head->dev);
> +			if (unlikely(err))
> +				goto out_rcu_unlock;
> +
> +			/*
> +			 * Only an end host needs to send an ICMP
> +			 * "Fragment Reassembly Timeout" message, per RFC792.
> +			 */
> +			if (skb_rtable(head)->rt_type != RTN_LOCAL) {
> +				skb_dst_drop(head);

Is manually dropping the dst entry necessary here? It will get released
by the fragment destructor anyways if I'm not mistaken.

> +				goto out_rcu_unlock;
> +			}
> +		}
> +
> +		/* Send an ICMP "Fragment Reassembly Timeout" message. */
> +		icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
>  	}
> +
> +out_rcu_unlock:
> +	rcu_read_unlock();
>  out:
>  	spin_unlock(&qp->q.lock);
>  	ipq_put(qp);

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Shan Wei]

Patrick McHardy wrote, at 01/21/2010 08:13 PM:
>> +			if (skb_rtable(head)->rt_type != RTN_LOCAL) {
>> +				skb_dst_drop(head);
> 
> Is manually dropping the dst entry necessary here? It will get released
> by the fragment destructor anyways if I'm not mistaken.

Yes, you are right.

--
[PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

No matter whether connection track is enabled, an end host should send 
an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
The reasons are following two points:  

1. RFC 792 says:
   >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
   >>>> >> > >   reassembly due to missing fragments within its time limit it
   >>>> >> > >   discards the datagram, and it may send a time exceeded message.
   >>>> >> > > 
   >>>> >> > >   If fragment zero is not available then no time exceeded need be
   >>>> >> > >   sent at all.
   >>>> >> > > 
   >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp

2. Patrick McHardy also agrees with this opinion.   :-)   
   About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649

The patch fixed the problem like this:
When enabling connection track, fragments are received at PRE_ROUTING HOOK.
If they are failed to reassemble, ip_expire() will be called. 
Before sending an ICMP "Fragment Reassembly Timeout" message, 
the patch searches router table to get the destination entry only for host type.

The patch has been tested on both host type and route type.


Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com> 
---
 net/ipv4/ip_fragment.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 86964b3..19aeef4 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -32,6 +32,8 @@
 #include <linux/netdevice.h>
 #include <linux/jhash.h>
 #include <linux/random.h>
+#include <net/route.h>
+#include <net/dst.h>
 #include <net/sock.h>
 #include <net/ip.h>
 #include <net/icmp.h>
@@ -205,13 +207,37 @@ static void ip_expire(unsigned long arg)
 	if ((qp->q.last_in & INET_FRAG_FIRST_IN) && qp->q.fragments != NULL) {
 		struct sk_buff *head = qp->q.fragments;
 
-		/* Send an ICMP "Fragment Reassembly Timeout" message. */
 		rcu_read_lock();
 		head->dev = dev_get_by_index_rcu(net, qp->iif);
-		if (head->dev)
-			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
-		rcu_read_unlock();
+		if (!head->dev)
+			goto out_rcu_unlock;
+
+		/*
+		 * Only search router table for the head fragment,
+		 * when defraging timeout at PRE_ROUTING HOOK.
+		 */
+		if (qp->user == IP_DEFRAG_CONNTRACK_IN && !skb_dst(head)) {
+			const struct iphdr *iph = ip_hdr(head);
+			int err = ip_route_input(head, iph->daddr, iph->saddr,
+						 iph->tos, head->dev);
+			if (unlikely(err))
+				goto out_rcu_unlock;
+
+			/*
+			 * Only an end host needs to send an ICMP
+			 * "Fragment Reassembly Timeout" message, per RFC792.
+			 */
+			if (skb_rtable(head)->rt_type != RTN_LOCAL)
+				goto out_rcu_unlock;
+
+		}
+
+		/* Send an ICMP "Fragment Reassembly Timeout" message. */
+		icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
 	}
+
+out_rcu_unlock:
+	rcu_read_unlock();
 out:
 	spin_unlock(&qp->q.lock);
 	ipq_put(qp);
--
1.6.3.3 

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Patrick McHardy]

Shan Wei wrote:
> [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track
> 
> No matter whether connection track is enabled, an end host should send 
> an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
> The reasons are following two points:  
> 
> 1. RFC 792 says:
>    >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
>    >>>> >> > >   reassembly due to missing fragments within its time limit it
>    >>>> >> > >   discards the datagram, and it may send a time exceeded message.
>    >>>> >> > > 
>    >>>> >> > >   If fragment zero is not available then no time exceeded need be
>    >>>> >> > >   sent at all.
>    >>>> >> > > 
>    >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp
> 
> 2. Patrick McHardy also agrees with this opinion.   :-)   
>    About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649
> 
> The patch fixed the problem like this:
> When enabling connection track, fragments are received at PRE_ROUTING HOOK.
> If they are failed to reassemble, ip_expire() will be called. 
> Before sending an ICMP "Fragment Reassembly Timeout" message, 
> the patch searches router table to get the destination entry only for host type.
> 
> The patch has been tested on both host type and route type.

Looks good to me. Would you mind adding a similar change to IPv6
(net/ipv6/netfilter/nf_conntrack_reasm.c)?

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 22 Jan 2010 12:48:20 +0100

> Shan Wei wrote:
>> [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track
 ...
>> The patch has been tested on both host type and route type.
> 
> Looks good to me. Would you mind adding a similar change to IPv6
> (net/ipv6/netfilter/nf_conntrack_reasm.c)?

Applied to net-next-2.6, thanks.

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Yasuyuki KOZAKAI]

Hi,

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 22 Jan 2010 12:48:20 +0100

> Shan Wei wrote:
> > [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track
> > 
> > No matter whether connection track is enabled, an end host should send 
> > an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
> > The reasons are following two points:  
> > 
> > 1. RFC 792 says:
> >    >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
> >    >>>> >> > >   reassembly due to missing fragments within its time limit it
> >    >>>> >> > >   discards the datagram, and it may send a time exceeded message.
> >    >>>> >> > > 
> >    >>>> >> > >   If fragment zero is not available then no time exceeded need be
> >    >>>> >> > >   sent at all.
> >    >>>> >> > > 
> >    >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp
> > 
> > 2. Patrick McHardy also agrees with this opinion.   :-)   
> >    About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649
> > 
> > The patch fixed the problem like this:
> > When enabling connection track, fragments are received at PRE_ROUTING HOOK.
> > If they are failed to reassemble, ip_expire() will be called. 
> > Before sending an ICMP "Fragment Reassembly Timeout" message, 
> > the patch searches router table to get the destination entry only for host type.
> > 
> > The patch has been tested on both host type and route type.
> 
> Looks good to me. Would you mind adding a similar change to IPv6
> (net/ipv6/netfilter/nf_conntrack_reasm.c)?

It sounds good. Please take care that IPv6 router does not reassemble
fragmented packets. IIRC the current nf_conntrack_{ipv6,reasm}.c
reassembles the cloned skbs for tracking, discard the cloned skbs after
tracking and forward the original skbs to IPv6 stack to keep the size of
fragmented packets.

-- Yasuyuki Kozakai

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Shan Wei]

Patrick McHardy wrote, at 01/22/2010 07:48 PM:
> Looks good to me. Would you mind adding a similar change to IPv6
> (net/ipv6/netfilter/nf_conntrack_reasm.c)?

Sorry to reply to you so late. 
I'm grade to send a patch to change IPv6 conntrack.

I also find that reassembly queues of IPv6 conntrack are broken 
when be initialed by ip6_frag_init(). After testing, I will send all patches.

-- 
Best Regards
-----
Shan Wei

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Shan Wei]

Yasuyuki KOZAKAI wrote, at 01/25/2010 08:57 AM:
> It sounds good. Please take care that IPv6 router does not reassemble
> fragmented packets. 

I don't know the details about IPv6 router implement.
Did you mean that we can not directly use ip6_route_input(skb) to find Routing type(host/router)? 

> IIRC the current nf_conntrack_{ipv6,reasm}.c
> reassembles the cloned skbs for tracking, discard the cloned skbs after
> tracking and forward the original skbs to IPv6 stack to keep the size of
> fragmented packets.

Indeed, after assembling fragments successfully in IPv6 connection track, original fragments are forwarded to IPv6 stack. And then IPv6 stack also assembles those received fragments again.
Thus fragments are assembled twice. 

But IPv4 only assembly once. IPv4 connection track assembles fragments successfully and then just forwards assembled intact packet to IPv4 stack. 
Do you know why is IPv6 designed like that?


-- 
Best Regards
-----
Shan Wei

Re: [PATCH v2]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

[Yasuyuki KOZAKAI]

Hi,

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Tue, 26 Jan 2010 09:25:54 +0800

> Yasuyuki KOZAKAI wrote, at 01/25/2010 08:57 AM:
> > It sounds good. Please take care that IPv6 router does not reassemble
> > fragmented packets. 
> 
> I don't know the details about IPv6 router implement.
> Did you mean that we can not directly use ip6_route_input(skb) to find
> Routing type(host/router)? 

I just talked about RFC2460.

4.5  Fragment Header

   The Fragment header is used by an IPv6 source to send a packet larger
   than would fit in the path MTU to its destination.  (Note: unlike
   IPv4, fragmentation in IPv6 is performed only by source nodes, not by
   routers along a packet's delivery path -- see section 5.) 


> > IIRC the current nf_conntrack_{ipv6,reasm}.c
> > reassembles the cloned skbs for tracking, discard the cloned skbs after
> > tracking and forward the original skbs to IPv6 stack to keep the size of
> > fragmented packets.
> 
> Indeed, after assembling fragments successfully in IPv6 connection track,
> original fragments are forwarded to IPv6 stack. And then IPv6 stack
> also assembles those received fragments again.
> Thus fragments are assembled twice. 

Yes, in the case that Linux is IPv6 host.


> But IPv4 only assembly once. IPv4 connection track assembles fragments
> > successfully and then just forwards assembled intact packet to IPv4
> > stack. 
> Do you know why is IPv6 designed like that?

General speaking, IPv6 router just forwards the fragmented packets and
it's up to IPv6 host to handle them. And nf_conntrack is not packet filter,
but it just tracks packets. So I designed that nf_contrack_ipv6 forwards
fragments to IPv6 stack even if nf_conntrack detects missing piece
of fragments. This resulted in twice reassembly in the case that
Linux is IPv6 host, but I tolerated that.

I think that your improvement can remove such inefficient processing.

BTW, I explained the reason why nf_conntrack does not re-fragment
the reassembled skb like nf_conntrack_ipv4 and forwards the original
fragments to IPv6 stack in

	http://conferences.sigcomm.org/sigcomm/2007/ipv6/1569042997.pdf

Regards,

-- Yasuyuki Kozakai