Re: [PATCH]ip_options_fragment() has no effect on fragmentation

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[Eric Dumazet]

Wei Yongjun a écrit :
> [1]Summary of the problem:
> ip_options_fragment() has no effect on fragmentation
> 
> [2]Full description of the problem:
> When I send IPv4 packet(contain Record Route Option) which need to be 
> fragmented to the router, the router can not fragment it correctly.
> After fragmented by router, the second fragmentation still contain 
> Record Route Option. Refer to RFC791, Record Route Option must Not be 
> copied on fragmentation, goes in first fragment only.
> ip_options_fragment() is the implemental function, but there are some 
> BUGs in it:
> 

Hello Wei

This is the third time I receive this patch (and I am not able to comment it), 
and still you didnt send it to the right list.

Please use netdev@vger.kernel.org for any linux kernel networking stuff.

If you dont receive an acknowledge from this netdev list, please wait at least 
two weeks before resending it.

Thank you
Eric

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[Patrick McHardy]

Wei Yongjun wrote:
> [1]Summary of the problem:
> ip_options_fragment() has no effect on fragmentation
> 
> [2]Full description of the problem:
> When I send IPv4 packet(contain Record Route Option) which need to be
> fragmented to the router, the router can not fragment it correctly.
> After fragmented by router, the second fragmentation still contain
> Record Route Option. Refer to RFC791, Record Route Option must Not be
> copied on fragmentation, goes in first fragment only.
> ip_options_fragment() is the implemental function, but there are some
> BUGs in it:
> 
> ip_option.c: line 207:
> void ip_options_fragment(struct sk_buff * skb)
> {
> unsigned char * optptr = skb->nh.raw;
> struct ip_options * opt = &(IPCB(skb)->opt);
> ...
> 
> optptr get a error pointer to the ipv4 options, correct is as following:
> 
> unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);
> 
> By the way, ip_options_fragment() just fill options not allowed in
> fragments with NOOPs, does not delete the space of the options,
> following patch has corrected the problem.

Please split the optptr fix and your enhancements in two patches and
send them to netdev@vger.kernel.org.

BTW, your mailer corrupts whitespace.

[PATCH]ip_options_fragment() has no effect on fragmentation

[Wei Yongjun]

[-- Attachment #1: Type: text/plain, Size: 4301 bytes --]

[1]Summary of the problem:
ip_options_fragment() has no effect on fragmentation

[2]Full description of the problem:
When I send IPv4 packet(contain Record Route Option) which need to be 
fragmented to the router, the router can not fragment it correctly.
After fragmented by router, the second fragmentation still contain Record 
Route Option. Refer to RFC791, Record Route Option must Not be copied on 
fragmentation, goes in first fragment only.
ip_options_fragment() is the implemental function, but there are some BUGs 
in it:

ip_option.c: line 207:
void ip_options_fragment(struct sk_buff * skb)
{
 unsigned char * optptr = skb->nh.raw;
 struct ip_options * opt = &(IPCB(skb)->opt);
...

optptr get a error pointer to the ipv4 options, correct is as following:

unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);

By the way, ip_options_fragment() just fill options not allowed in fragments 
with NOOPs, does not delete the space of the options, following patch has 
corrected the problem.

--- linux-2.6.15.1/net/ipv4/ip_options.c.orig 2006-01-23 08:55:15.045904912 
+0900
+++ linux-2.6.15.1/net/ipv4/ip_options.c 2006-01-23 09:04:28.885708464 +0900
@@ -207,33 +207,63 @@

 void ip_options_fragment(struct sk_buff * skb)
 {
- unsigned char * optptr = skb->nh.raw;
+ unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);
  struct ip_options * opt = &(IPCB(skb)->opt);
  int  l = opt->optlen;
  int  optlen;
+ int  optneed = 0;
+ unsigned char * pp_ptr = optptr;

  while (l > 0) {
   switch (*optptr) {
   case IPOPT_END:
-   return;
+   goto end;
   case IPOPT_NOOP:
    l--;
+   if(optptr != pp_ptr)
+    memcpy(pp_ptr, optptr, 1);
    optptr++;
+   pp_ptr++;
    continue;
   }
   optlen = optptr[1];
-  if (optlen<2 || optlen>l)
-    return;
-  if (!IPOPT_COPIED(*optptr))
-   memset(optptr, IPOPT_NOOP, optlen);
+  if (optlen<2 || optlen>l) {
+   if(optptr != pp_ptr)
+    memcpy(pp_ptr, optptr, l);
+   optptr += l;
+   pp_ptr += l;
+   optneed = 1;
+   goto error;
+  }
+  if (IPOPT_COPIED(*optptr)) {
+   if(optptr != pp_ptr)
+    memcpy(pp_ptr, optptr, optlen);
+   pp_ptr += optlen;
+   optneed = 1;
+  }
   l -= optlen;
   optptr += optlen;
  }
+end:
  opt->ts = 0;
  opt->rr = 0;
  opt->rr_needaddr = 0;
  opt->ts_needaddr = 0;
  opt->ts_needtime = 0;
+error:
+ if (pp_ptr != optptr) {
+  if (optneed == 1) {
+   opt->optlen -= optptr - pp_ptr;
+   if (opt->optlen & 0x03) {
+    for (l = 0; l < 4 - (opt->optlen & 0x03); l++)
+     *pp_ptr++ = IPOPT_END;
+    opt->optlen = (opt->optlen + 3) & ~3;
+   }
+  } else {
+   opt->optlen = 0;
+  }
+  skb->nh.iph->ihl = 5 + (opt->optlen >> 2);
+ }
  return;
 }

--- linux-2.6.15.1/net/ipv4/ip_output.c.orig 2006-01-23 08:54:57.516569776 
+0900
+++ linux-2.6.15.1/net/ipv4/ip_output.c 2006-01-23 09:09:13.531435736 +0900
@@ -503,12 +503,19 @@
     frag->h.raw = frag->data;
     frag->nh.raw = __skb_push(frag, hlen);
     memcpy(frag->nh.raw, iph, hlen);
+    offset += skb->len - hlen;
+    if (offset == skb->len - hlen) {
+     ip_options_fragment(frag);
+     len = frag->nh.iph->ihl * 4;
+     if (hlen != len) {
+      memmove(frag->nh.raw, frag->h.raw - len, len);
+      frag->nh.raw = __skb_pull(frag, hlen - len);
+      hlen = len;
+     }
+    }
     iph = frag->nh.iph;
     iph->tot_len = htons(frag->len);
     ip_copy_metadata(frag, skb);
-    if (offset == 0)
-     ip_options_fragment(frag);
-    offset += skb->len - hlen;
     iph->frag_off = htons(offset>>3);
     if (frag->next != NULL)
      iph->frag_off |= htons(IP_MF);
@@ -619,6 +626,7 @@
    */
   iph = skb2->nh.iph;
   iph->frag_off = htons((offset >> 3));
+  iph->tot_len = htons(len + hlen);

   /* ANK: dirty, but effective trick. Upgrade options only if
    * the segment to be fragmented was THE FIRST (otherwise,
@@ -626,8 +634,11 @@
    * on the initial skb, so that all the following fragments
    * will inherit fixed options.
    */
-  if (offset == 0)
+  if (offset == 0) {
    ip_options_fragment(skb);
+   hlen = skb->nh.iph->ihl * 4;
+   mtu = dst_pmtu(&rt->u.dst) - hlen;
+  }

   /*
    * Added AC : If we are fragmenting a fragment that's not the
@@ -644,8 +655,6 @@

   IP_INC_STATS(IPSTATS_MIB_FRAGCREATES);

-  iph->tot_len = htons(len + hlen);
-
   ip_send_check(iph);

   err = output(skb2);

Best Regard
Wei Yongjun

[-- Attachment #2: fix-a-bug-in-ip_options_fragment.patch --]
[-- Type: application/octet-stream, Size: 3402 bytes --]

--- linux-2.6.15.1/net/ipv4/ip_options.c.orig	2006-01-23 08:55:15.045904912 +0900
+++ linux-2.6.15.1/net/ipv4/ip_options.c	2006-01-23 09:04:28.885708464 +0900
@@ -207,33 +207,63 @@
 
 void ip_options_fragment(struct sk_buff * skb) 
 {
-	unsigned char * optptr = skb->nh.raw;
+	unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);
 	struct ip_options * opt = &(IPCB(skb)->opt);
 	int  l = opt->optlen;
 	int  optlen;
+	int  optneed = 0;
+	unsigned char * pp_ptr = optptr;
 
 	while (l > 0) {
 		switch (*optptr) {
 		case IPOPT_END:
-			return;
+			goto end;
 		case IPOPT_NOOP:
 			l--;
+			if(optptr != pp_ptr)
+				memcpy(pp_ptr, optptr, 1);
 			optptr++;
+			pp_ptr++;
 			continue;
 		}
 		optlen = optptr[1];
-		if (optlen<2 || optlen>l)
-		  return;
-		if (!IPOPT_COPIED(*optptr))
-			memset(optptr, IPOPT_NOOP, optlen);
+		if (optlen<2 || optlen>l) {
+			if(optptr != pp_ptr)
+				memcpy(pp_ptr, optptr, l);
+			optptr += l;
+			pp_ptr += l;
+			optneed = 1;
+			goto error;
+		}
+		if (IPOPT_COPIED(*optptr)) {
+			if(optptr != pp_ptr)
+				memcpy(pp_ptr, optptr, optlen);
+			pp_ptr += optlen;
+			optneed = 1;
+		}
 		l -= optlen;
 		optptr += optlen;
 	}
+end:
 	opt->ts = 0;
 	opt->rr = 0;
 	opt->rr_needaddr = 0;
 	opt->ts_needaddr = 0;
 	opt->ts_needtime = 0;
+error:
+	if (pp_ptr != optptr) {
+		if (optneed == 1) {
+			opt->optlen -= optptr - pp_ptr;
+			if (opt->optlen & 0x03) {
+				for (l = 0; l < 4 - (opt->optlen & 0x03); l++)
+					*pp_ptr++ = IPOPT_END;
+				opt->optlen = (opt->optlen + 3) & ~3;
+			}
+		} else {
+			opt->optlen = 0;
+		}
+		skb->nh.iph->ihl = 5 + (opt->optlen >> 2);
+	}
 	return;
 }
 
--- linux-2.6.15.1/net/ipv4/ip_output.c.orig	2006-01-23 08:54:57.516569776 +0900
+++ linux-2.6.15.1/net/ipv4/ip_output.c	2006-01-23 09:09:13.531435736 +0900
@@ -503,12 +503,19 @@
 				frag->h.raw = frag->data;
 				frag->nh.raw = __skb_push(frag, hlen);
 				memcpy(frag->nh.raw, iph, hlen);
+				offset += skb->len - hlen;
+				if (offset == skb->len - hlen) {
+					ip_options_fragment(frag);
+					len = frag->nh.iph->ihl * 4;
+					if (hlen != len) {
+						memmove(frag->nh.raw, frag->h.raw - len, len);
+						frag->nh.raw = __skb_pull(frag, hlen - len);
+						hlen = len;
+					}
+				}
 				iph = frag->nh.iph;
 				iph->tot_len = htons(frag->len);
 				ip_copy_metadata(frag, skb);
-				if (offset == 0)
-					ip_options_fragment(frag);
-				offset += skb->len - hlen;
 				iph->frag_off = htons(offset>>3);
 				if (frag->next != NULL)
 					iph->frag_off |= htons(IP_MF);
@@ -619,6 +626,7 @@
 		 */
 		iph = skb2->nh.iph;
 		iph->frag_off = htons((offset >> 3));
+		iph->tot_len = htons(len + hlen);
 
 		/* ANK: dirty, but effective trick. Upgrade options only if
 		 * the segment to be fragmented was THE FIRST (otherwise,
@@ -626,8 +634,11 @@
 		 * on the initial skb, so that all the following fragments
 		 * will inherit fixed options.
 		 */
-		if (offset == 0)
+		if (offset == 0) {
 			ip_options_fragment(skb);
+			hlen = skb->nh.iph->ihl * 4;
+			mtu = dst_pmtu(&rt->u.dst) - hlen;
+		}
 
 		/*
 		 *	Added AC : If we are fragmenting a fragment that's not the
@@ -644,8 +655,6 @@
 
 		IP_INC_STATS(IPSTATS_MIB_FRAGCREATES);
 
-		iph->tot_len = htons(len + hlen);
-
 		ip_send_check(iph);
 
 		err = output(skb2);

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[weiyj]

Hello Mr. David:

Does this patch will be used? 

This patch resolved the following problem: When I send IPv4 packet(contain 
Record Route Option) which need to be fragmented to the router, the router 
can not fragment it correctly. After fragmented by router, the second 
fragmentation still contain Record Route Option. Refer to RFC791, Record 
Route Option must Not be copied on fragmentation, goes in first fragment 
only.

On Thursday 02 February 2006 20:08, David S. Miller wrote:
> From: Wei Yongjun <weiyj@soft.fujitsu.com>
> Date: Wed, 01 Feb 2006 14:21:41 -0500
>
> Your patch is still corrupt, new lines were added by your email client
> which splits up the patch headers.
>
> I applied the patch by hand, but next time I won't put so much effort
> into fixing up your work.  Please learn how to submit patches
> properly.
>
> Thank you.

--- a/net/ipv4/ip_options.c.orig 2006-01-27 09:14:33.463612696 +0900
+++ b/net/ipv4/ip_options.c 2006-01-27 09:12:21.857619848 +0900
@@ -207,7 +207,7 @@
 
 void ip_options_fragment(struct sk_buff * skb) 
 {
- unsigned char * optptr = skb->nh.raw;
+ unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);
  struct ip_options * opt = &(IPCB(skb)->opt);
  int  l = opt->optlen;
  int  optlen;

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[Wei Yongjun]

I had tested the patch under linux system, maybe this mail is correct.

Fix error point to options in ip_options_fragment(). optptr get a error
pointer to the ipv4 header, correct is pointer to ipv4 options.

Signed-off-by: Wei Yongjun <weiyj@soft.fujitsu.com>

--- a/net/ipv4/ip_options.c	2006-01-27 09:14:33.463612696 +0900
+++ b/net/ipv4/ip_options.c	2006-01-27 09:12:21.857619848 +0900
@@ -207,7 +207,7 @@
 
 void ip_options_fragment(struct sk_buff * skb) 
 {
-	unsigned char * optptr = skb->nh.raw;
+	unsigned char * optptr = skb->nh.raw + sizeof(struct iphdr);
 	struct ip_options * opt = &(IPCB(skb)->opt);
 	int  l = opt->optlen;
 	int  optlen;


On Monday 08 May 2006 01:41, David S. Miller wrote:
> Actually it didn't get applied for some reason.
>
> Please resubmit it properly with a full changelog and
> "Signed-off-by: " lines, and make double sure that your
> email client does not corrupt the patch so that I may
> apply it cleanly.  Test this by sending it to yourself
> and trying to apply the patch.

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[David S. Miller]

From: weiyj@soft.fujitsu.com
Date: Fri, 5 May 2006 13:50:02 -0400

> Does this patch will be used? 

Your patch is in the tree already.

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[David S. Miller]

From: weiyj@soft.fujitsu.com
Date: Fri, 5 May 2006 13:50:02 -0400

> Does this patch will be used? 
> 
> This patch resolved the following problem: When I send IPv4 packet(contain 
> Record Route Option) which need to be fragmented to the router, the router 
> can not fragment it correctly. After fragmented by router, the second 
> fragmentation still contain Record Route Option. Refer to RFC791, Record 
> Route Option must Not be copied on fragmentation, goes in first fragment 
> only.

Actually it didn't get applied for some reason.

Please resubmit it properly with a full changelog and
"Signed-off-by: " lines, and make double sure that your
email client does not corrupt the patch so that I may
apply it cleanly.  Test this by sending it to yourself
and trying to apply the patch.

Thanks.

Re: [PATCH]ip_options_fragment() has no effect on fragmentation

[David S. Miller]

From: Wei Yongjun <weiyj@soft.fujitsu.com>
Date: Fri, 05 May 2006 20:36:14 -0400

> I had tested the patch under linux system, maybe this mail is correct.
> 
> Fix error point to options in ip_options_fragment(). optptr get a error
> pointer to the ipv4 header, correct is pointer to ipv4 options.
> 
> Signed-off-by: Wei Yongjun <weiyj@soft.fujitsu.com>

Patch applied, thank you very much.