tunnel xmit and h.raw

tunnel xmit and h.raw

[Julian Anastasov]

	Hello,

	Is the following change needed? May be yes for all kernels
where the nearest skb_shared checks are actual.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1356  -> 1.1357 
#	   net/ipv4/ip_gre.c	1.30    -> 1.31   
#	      net/ipv6/sit.c	1.28    -> 1.29   
#	     net/ipv4/ipip.c	1.32    -> 1.33   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/03	ja@ssi.bg	1.1357
# [IPV4/IPV6]: tunnel xmit must load skb->h.raw after all reallocations
# --------------------------------------------
#
diff -Nru a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
--- a/net/ipv4/ip_gre.c	Fri Oct  3 02:43:29 2003
+++ b/net/ipv4/ip_gre.c	Fri Oct  3 02:43:29 2003
@@ -803,8 +803,6 @@
 			tunnel->err_count = 0;
 	}
 
-	skb->h.raw = skb->nh.raw;
-
 	max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen;
 
 	if (skb_headroom(skb) < max_headroom || skb_cloned(skb) || skb_shared(skb)) {
@@ -823,6 +821,7 @@
 		old_iph = skb->nh.iph;
 	}
 
+	skb->h.raw = skb->nh.raw;
 	skb->nh.raw = skb_push(skb, gre_hlen);
 	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
 	dst_release(skb->dst);
diff -Nru a/net/ipv4/ipip.c b/net/ipv4/ipip.c
--- a/net/ipv4/ipip.c	Fri Oct  3 02:43:29 2003
+++ b/net/ipv4/ipip.c	Fri Oct  3 02:43:29 2003
@@ -596,8 +596,6 @@
 			tunnel->err_count = 0;
 	}
 
-	skb->h.raw = skb->nh.raw;
-
 	/*
 	 * Okay, now see if we can stuff it in the buffer as-is.
 	 */
@@ -619,6 +617,7 @@
 		old_iph = skb->nh.iph;
 	}
 
+	skb->h.raw = skb->nh.raw;
 	skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
 	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
 	dst_release(skb->dst);
diff -Nru a/net/ipv6/sit.c b/net/ipv6/sit.c
--- a/net/ipv6/sit.c	Fri Oct  3 02:43:29 2003
+++ b/net/ipv6/sit.c	Fri Oct  3 02:43:29 2003
@@ -530,8 +530,6 @@
 			tunnel->err_count = 0;
 	}
 
-	skb->h.raw = skb->nh.raw;
-
 	/*
 	 * Okay, now see if we can stuff it in the buffer as-is.
 	 */
@@ -553,6 +551,7 @@
 		iph6 = skb->nh.ipv6h;
 	}
 
+	skb->h.raw = skb->nh.raw;
 	skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
 	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
 	dst_release(skb->dst);


Regards

--
Julian Anastasov <ja@ssi.bg>

Re: tunnel xmit and h.raw

[David S. Miller]

On Fri, 3 Oct 2003 03:00:27 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> 	Is the following change needed? May be yes for all kernels
> where the nearest skb_shared checks are actual.

There seems to be no end to these kinds of bugs in the tunnel
drivers!

Didn't we fix a nearly identical set of bugs in the tunnel
drivers just a month or two ago?

I'll review this some more tomorrow and unless I find some problem
I'll apply your patch.  Thanks a lot.

Re: tunnel xmit and h.raw

[Julian Anastasov]

	Hello,

On Fri, 3 Oct 2003, David S. Miller wrote:

> > 	Is the following change needed? May be yes for all kernels
> > where the nearest skb_shared checks are actual.
>
> There seems to be no end to these kinds of bugs in the tunnel
> drivers!
>
> Didn't we fix a nearly identical set of bugs in the tunnel
> drivers just a month or two ago?

	The same place, but this one is special: h.raw is updated
for us on reallocation but if the skb is shared I do not know if this is
fatal for the other skb users, we change also their h.raw. But I rely
on your opinion, may be it is safer with this change.

> I'll review this some more tomorrow and unless I find some problem
> I'll apply your patch.  Thanks a lot.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: tunnel xmit and h.raw

[David S. Miller]

On Fri, 3 Oct 2003 17:56:35 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> On Fri, 3 Oct 2003, David S. Miller wrote:
> 
> > Didn't we fix a nearly identical set of bugs in the tunnel
> > drivers just a month or two ago?
> 
> 	The same place, but this one is special: h.raw is updated
> for us on reallocation but if the skb is shared I do not know if this is
> fatal for the other skb users, we change also their h.raw. But I rely
> on your opinion, may be it is safer with this change.

Oh, I see, the code is actually not buggy as-is.  The following two
sequences are identical:

1)	skb->h.raw = skb->nh.raw;
	new_skb = skb_realloc_headroom(...);
	skb = new_skb;

2)	new_skb = skb_realloc_headroom(...);
	skb = new_skb;
	skb->h.raw = skb->nh.raw;

Because skb_realloc_headroom() keeps all the skb->*.raw pointers
in the right relative place if it allocates new data pointers or
whatever.

But the code is certainly clearer to read with your changes so I'll
add them as a code cleanup instead of a bug fix :)

Re: tunnel xmit and h.raw

[David S. Miller]

On Fri, 3 Oct 2003 17:56:35 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> 	The same place, but this one is special: h.raw is updated
> for us on reallocation but if the skb is shared I do not know if this is
> fatal for the other skb users, we change also their h.raw.

Sorry, replying again... I hadn't read your reply correctly.

Indeed, I have to think about what changing skb->h.raw might do
to other users, it might indeed be an issue.

Re: tunnel xmit and h.raw

[Mika Penttilä]

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

If skb is shared skb_realloc_headroom() makes a private head, so no 
problem here. Actually the skb_cloned() test seems redundant in this 
context.

--Mika


Julian Anastasov wrote:

>	Hello,
>
>On Fri, 3 Oct 2003, David S. Miller wrote:
>
>  
>
>>>	Is the following change needed? May be yes for all kernels
>>>where the nearest skb_shared checks are actual.
>>>      
>>>
>>There seems to be no end to these kinds of bugs in the tunnel
>>drivers!
>>
>>Didn't we fix a nearly identical set of bugs in the tunnel
>>drivers just a month or two ago?
>>    
>>
>
>	The same place, but this one is special: h.raw is updated
>for us on reallocation but if the skb is shared I do not know if this is
>fatal for the other skb users, we change also their h.raw. But I rely
>on your opinion, may be it is safer with this change.
>
>  
>
>>I'll review this some more tomorrow and unless I find some problem
>>I'll apply your patch.  Thanks a lot.
>>    
>>
>
>Regards
>
>--
>Julian Anastasov <ja@ssi.bg>
>
>
>
>  
>

[-- Attachment #2: Type: text/html, Size: 1637 bytes --]

Re: tunnel xmit and h.raw

[David S. Miller]

On Fri, 3 Oct 2003 03:00:27 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> # --------------------------------------------
> # 03/10/03	ja@ssi.bg	1.1357
> # [IPV4/IPV6]: tunnel xmit must load skb->h.raw after all reallocations
> # --------------------------------------------

I've applied this, it is absolutely correct.

While studying this I've discovered some deeper problems.
The skb_shared() checks in these tunnel drivers is bogus,
on transmit skb->users should never ever be anything but 1.

Besides the tunnel drivers, the loopback driver and ipmr.c
do similar things in their ->hard_start_xmit() handlers.
In particular, the ipmr.c case is really in bad shape.

I've put fixing this onto the todo list...