[PATCH][2.5.22] OOPS in tcp_v6_get_port

[PATCH][2.5.22] OOPS in tcp_v6_get_port

[Carl Ritson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2454 bytes --]


Sorry for the repeat email but this bug is also in 2.5.22 and my patch is still
valid, although I'm not entirely sure it is the correct fix for the problem?

-- From Previous email --
2.5.21 and 2.5.22 OOPS at boot on my test machine, the decoded OOPS is attached.
Also attached is a program that triggers the bug, it emulates the behavior 
of bind on my test machine and binds to two ports one IPv4 and one IPv6 
with the same port number but different IP addresses.

The bug appears to be the IPv6 TCP code, in net/ipv6/tcp_ipv6.c
Line 149:
	struct ipv6_pinfo *np2 = inet6_sk(sk2);

	if (sk != sk2 &&
	    sk->bound_dev_if == sk2->bound_dev_if) {
		if (!sk_reuse	||
		    !sk2->reuse	||
		    sk2->state == TCP_LISTEN) {
			/* NOTE: IPv6 tw bucket have different format */
			if (!inet_sk(sk2)->rcv_saddr ||
			    addr_type == IPV6_ADDR_ANY ||
			    !ipv6_addr_cmp(&np->rcv_saddr,
			    		sk2->state != TCP_TIME_WAIT ?
BUG -->					&np2->rcv_saddr :
					&((struct tcp_tw_bucket*)sk)->v6_rcv_saddr) ||
			    (addr_type==IPV6_ADDR_MAPPED && sk2->family==AF_INET &&
			     inet_sk(sk)->rcv_saddr ==
			     inet_sk(sk2)->rcv_saddr))
				break;
			}
		}
	}

np2 can be NULL if the socket is an IPv4 socket, since IPv6 and IPv4 
share the port address space.  While the test of !inet_sk(sk2-)->rcv_addr 
_should_ prevent this it is not assured in C that the conditions to an if 
statement will be evaluated in the order written?  At least my gcc 
(2.95.4 from compiled from CVS) doesn't think so :-).

I propose a diff similar to the one below to fix the problem maybe?

Many thanks,
Carl Ritson
critson@perlfu.co.uk

--- orig/net/ipv6/tcp_ipv6.c	Sat Jun 15 19:23:44 2002
+++ linux/net/ipv6/tcp_ipv6.c	Sat Jun 15 19:21:46 2002
@@ -156,14 +156,16 @@
 						/* NOTE: IPv6 tw bucket have different format */
 						if (!inet_sk(sk2)->rcv_saddr ||
 						    addr_type == IPV6_ADDR_ANY ||
-						    !ipv6_addr_cmp(&np->rcv_saddr,
-								   sk2->state != TCP_TIME_WAIT ?
-								   &np2->rcv_saddr :
-								   &((struct tcp_tw_bucket*)sk)->v6_rcv_saddr) ||
 						    (addr_type==IPV6_ADDR_MAPPED && sk2->family==AF_INET &&
 						     inet_sk(sk)->rcv_saddr ==
 						     inet_sk(sk2)->rcv_saddr))
 							break;
+						if (np2 != NULL) 
+							if (!ipv6_addr_cmp(&np->rcv_saddr,
+								   sk2->state != TCP_TIME_WAIT ?
+								   &np2->rcv_saddr :
+								   &((struct tcp_tw_bucket*)sk)->v6_rcv_saddr))
+								break;
 					}
 				}
 			}


[-- Attachment #2: Type: TEXT/PLAIN, Size: 2217 bytes --]

ksymoops 2.4.4 on i686 2.4.19-pre8-ac2.  Options used
     -v /usr/src/linux-2.5.21/vmlinux (specified)
     -K (specified)
     -L (specified)
     -o /lib/modules/2.5.21/ (specified)
     -m /boot/System.map-2.5.21 (specified)

No modules in ksyms, skipping objects
Unable to handle kernel NULL pointer dereference at virtual address 00000010
c027244e
*pde = 00000000
Oops: 0000
CPU:    0
EIP:    0010:[<c027244e>]  Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010246
eax: cfd68b28   ebx: cea60360     ecx: 00000010       edx: cfd68a0a
esi: cfd68de8   edi: 00000010     ebp: cf9cf260       esp: ce8a7e98
ds: 0018   es: 0018   ss: 0018
Stack:  ce8a7f1c cfd68ac0 cfd68de8 c0341908 cfd68de8 0100007f 00000011 00000001
        cfd68ac0 cfd21dc8 03b93874 c0260f81 cfd68ac0 000003b9 ce8a7a40 ce8a7f14
        0000001c 40263874 00000011 03b919c8 0600007f cfd68dd8 cfd68bf8 c02227c0
Call Trace: [<c0260f81>] [<c02227c0>] [c0223629>] [<c0221cf5>] [<c0222d2e>]
   [<c0222d50>] [<c02232a0>] [<c0106a17>]
Code: f3 a6 74 2c 81 7c 24 18 00 10 00 00 75 17 66 83 7b 1c 02 75

>>EIP; c027244e <tcp_v6_get_port+1ae/2d4>   <=====
Trace; c0260f81 <inet6_bind+2b1/3d8>
Trace; c02227c0 <sys_bind+54/74>
Trace; c0222d50 <sys_setsockopt+6c/78>
Trace; c02232a0 <sys_socketcall+74/1fc>
Trace; c0106a17 <syscall_call+7/b>
Code;  c027244e <tcp_v6_get_port+1ae/2d4>
00000000 <_EIP>:
Code;  c027244e <tcp_v6_get_port+1ae/2d4>   <=====
   0:   f3 a6                     repz cmpsb %es:(%edi),%ds:(%esi)   <=====
Code;  c0272450 <tcp_v6_get_port+1b0/2d4>
   2:   74 2c                     je     30 <_EIP+0x30> c027247e <tcp_v6_get_port+1de/2d4>
Code;  c0272452 <tcp_v6_get_port+1b2/2d4>
   4:   81 7c 24 18 00 10 00      cmpl   $0x1000,0x18(%esp,1)
Code;  c0272459 <tcp_v6_get_port+1b9/2d4>
   b:   00 
Code;  c027245a <tcp_v6_get_port+1ba/2d4>
   c:   75 17                     jne    25 <_EIP+0x25> c0272473 <tcp_v6_get_port+1d3/2d4>
Code;  c027245c <tcp_v6_get_port+1bc/2d4>
   e:   66 83 7b 1c 02            cmpw   $0x2,0x1c(%ebx)
Code;  c0272461 <tcp_v6_get_port+1c1/2d4>
  13:   75 00                     jne    15 <_EIP+0x15> c0272463 <tcp_v6_get_port+1c3/2d4>


[-- Attachment #3: Type: TEXT/PLAIN, Size: 1593 bytes --]

#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>

const unsigned short bind_port = 4500;

int do_ipv4_bind(char *ip, const unsigned short port) {
	struct sockaddr_in addr;
	int sock;
	int ret;
	
	memset(&addr, 0x00, sizeof(addr));
	addr.sin_family = AF_INET;
	addr.sin_port = htons(port);
	inet_pton(AF_INET, ip, &(addr.sin_addr));
	
	ret = socket(PF_INET, SOCK_STREAM, 0);
	if(ret == -1) {
		return ret;
	} else {
		sock = ret;
	}
	ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
	if(ret == -1) {
		return ret;
	} else {
		return sock;
	}
}

int do_ipv6_bind(char *ip, const unsigned short port) {
	struct sockaddr_in6 addr;
	int sock;
	int ret;
	
	memset(&addr, 0x00, sizeof(addr));
	addr.sin6_family = AF_INET6;
	addr.sin6_port = htons(port);
	inet_pton(AF_INET6, ip, &(addr.sin6_addr));
	
	ret = socket(PF_INET6, SOCK_STREAM, 0);
	if(ret == -1) {
		return ret;
	} else {
		sock = ret;
	}

	ret = bind(sock, (struct sockaddr *)&addr, sizeof(addr));
	if(ret == -1) {
		return ret;
	} else {
		return sock;
	}
}

int main(void) {
	int ipv4_sock = 0;
	int ipv6_sock = 0;
	ipv4_sock = do_ipv4_bind("127.0.0.1", bind_port);
	ipv6_sock = do_ipv6_bind("::1", bind_port);
	if(ipv4_sock < 0) {
		printf("ipv4 bind failed\n");
	} else {
		printf("ipv4 bind succeeded\n");
		close(ipv4_sock);
	}
	if(ipv6_sock < 0) {
		printf("ipv6 bind failed\n");
	} else {
		printf("ipv6 bind succeeded\n");
		close(ipv6_sock);
	}
	return 0;
}

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

This is a known bug introduced by the struct sock splitup into
external per-protocol pieces done by Arnaldo de Melo.  He is working
on the proper fix, your proposed change will just paper over the real
bug.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 02:33:19PM -0700, David S. Miller escreveu:
> 
> This is a known bug introduced by the struct sock splitup into
> external per-protocol pieces done by Arnaldo de Melo.  He is working
> on the proper fix, your proposed change will just paper over the real
> bug.

Carl,

	Can you try this patch?

- Arnaldo

--- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
+++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
@@ -1240,6 +1240,7 @@
 					  struct dst_entry *dst)
 {
 	struct ipv6_pinfo *newnp, *np = inet6_sk(sk);
+	struct tcp6_sock *newtcp6sk;
 	struct flowi fl;
 	struct inet_opt *newinet;
 	struct tcp_opt *newtp;
@@ -1256,10 +1257,15 @@
 		if (newsk == NULL) 
 			return NULL;
 
+		newtcp6sk = (struct tcp6_sock *)newsk;
+		newtcp6sk->pinet6 = &newtcp6sk->inet6;
+
 		newinet = inet_sk(newsk);
 		newnp = inet6_sk(newsk);
 		newtp = tcp_sk(newsk);
 
+		memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 		ipv6_addr_set(&newnp->daddr, 0, 0, htonl(0x0000FFFF),
 			      newinet->daddr);
 
@@ -1336,9 +1342,15 @@
 	ip6_dst_store(newsk, dst, NULL);
 	sk->route_caps = dst->dev->features&~NETIF_F_IP_CSUM;
 
+	newtcp6sk = (struct tcp6_sock *)newsk;
+	newtcp6sk->pinet6 = &newtcp6sk->inet6;
+
 	newtp = tcp_sk(newsk);
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
+
+	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 	ipv6_addr_copy(&newnp->daddr, &req->af.v6_req.rmt_addr);
 	ipv6_addr_copy(&newnp->saddr, &req->af.v6_req.loc_addr);
 	ipv6_addr_copy(&newnp->rcv_saddr, &req->af.v6_req.loc_addr);


-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Mon, 17 Jun 2002 21:57:35 -0300

   --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
   +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002

I've installed this change into my tree in the meantime.
If we find a better fix, we can just revert this.

Thanks.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
>    Date: Mon, 17 Jun 2002 21:57:35 -0300
> 
>    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
>    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> 
> I've installed this change into my tree in the meantime.
> If we find a better fix, we can just revert this.

OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
as suggested by Russel King, I'll be sending RSN.

- Arnaldo

[BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Mon, Jun 17, 2002 at 11:49:34PM -0300, Arnaldo C. Melo escreveu:
> Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
> >    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
> >    Date: Mon, 17 Jun 2002 21:57:35 -0300
> > 
> >    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
> >    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> > 
> > I've installed this change into my tree in the meantime.
> > If we find a better fix, we can just revert this.
> 
> OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
> in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
> as suggested by Russel King, I'll be sending RSN.

Here it is, David, please consider pulling it from:

http://kernel-acme.bkbits.net:8080/tcpv6-pinet6

Best Regards,

- Arnaldo


# 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.656   -> 1.657  
#	 net/ipv6/tcp_ipv6.c	1.17    -> 1.18   
#	 net/ipv6/af_inet6.c	1.8     -> 1.9    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/06/18	acme@conectiva.com.br	1.657
# net/ipv6/af_inet6.c
# net/ipv6/tcp_ipv6.c
# 
# 	- Add constructors for the tcp6_sk_cachep, udp6_sk_cachep and raw6_sk_cachep slab caches,
#           to initialize ->pinet6 to the respective &->inet6, this simplifies inet6_create,
#           making ipv6_sk_generic not needed and fixing a bug where we create a new tcpv6 socket
#           outside inet6_create and we were not initializing ->pinet6 properly, thanks to
#           Russell King for pointing out the bug and doing the initial analisys that made the
#           fix easy to make.
# --------------------------------------------
#
diff -Nru a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
--- a/net/ipv6/af_inet6.c	Mon Mar 11 11:13:05 2002
+++ b/net/ipv6/af_inet6.c	Tue Jun 18 00:27:30 2002
@@ -134,23 +134,11 @@
         return rc;
 }
 
-static __inline__ struct ipv6_pinfo *inet6_sk_generic(struct sock *sk)
-{
-	struct ipv6_pinfo *rc = (&((struct tcp6_sock *)sk)->inet6);
-
-        if (sk->protocol == IPPROTO_UDP)
-                rc = (&((struct udp6_sock *)sk)->inet6);
-        else if (sk->protocol == IPPROTO_RAW)
-                rc = (&((struct raw6_sock *)sk)->inet6);
-        return rc;
-}
-
 static int inet6_create(struct socket *sock, int protocol)
 {
 	struct inet_opt *inet;
 	struct ipv6_pinfo *np;
 	struct sock *sk;
-	struct tcp6_sock* tcp6sk;
 	struct list_head *p;
 	struct inet_protosw *answer;
 
@@ -212,8 +200,7 @@
 
 	sk->backlog_rcv		= answer->prot->backlog_rcv;
 
-	tcp6sk		= (struct tcp6_sock *)sk;
-	tcp6sk->pinet6 = np = inet6_sk_generic(sk);
+	np		= inet6_sk(sk);
 	np->hop_limit	= -1;
 	np->mcast_hops	= -1;
 	np->mc_loop	= 1;
@@ -632,6 +619,30 @@
 	inet_unregister_protosw(p);
 }
 
+static void tcp6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct tcp6_sock *tcp6sk = (struct tcp6_sock *)foo;
+
+	tcp6sk->pinet6 = &tcp6sk->inet6;
+}
+
+static void udp6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct udp6_sock *udp6sk = (struct udp6_sock *)foo;
+
+	udp6sk->pinet6 = &udp6sk->inet6;
+}
+
+static void raw6_pinet6_init(void *foo, kmem_cache_t *cachep,
+			     unsigned long flags)
+{
+	struct raw6_sock *raw6sk = (struct raw6_sock *)foo;
+
+	raw6sk->pinet6 = &raw6sk->inet6;
+}
+
 static int __init inet6_init(void)
 {
 	struct sk_buff *dummy_skb;
@@ -655,13 +666,16 @@
 	/* allocate our sock slab caches */
         tcp6_sk_cachep = kmem_cache_create("tcp6_sock",
 					   sizeof(struct tcp6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   tcp6_pinet6_init, NULL);
         udp6_sk_cachep = kmem_cache_create("udp6_sock",
 					   sizeof(struct udp6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   udp6_pinet6_init, NULL);
         raw6_sk_cachep = kmem_cache_create("raw6_sock",
 					   sizeof(struct raw6_sock), 0,
-                                           SLAB_HWCACHE_ALIGN, 0, 0);
+                                           SLAB_HWCACHE_ALIGN,
+					   raw6_pinet6_init, NULL);
         if (!tcp6_sk_cachep || !udp6_sk_cachep || !raw6_sk_cachep)
                 printk(KERN_CRIT __FUNCTION__
                         ": Can't create protocol sock SLAB caches!\n");
diff -Nru a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
--- a/net/ipv6/tcp_ipv6.c	Wed May 22 15:16:37 2002
+++ b/net/ipv6/tcp_ipv6.c	Tue Jun 18 00:27:30 2002
@@ -1260,6 +1260,8 @@
 		newnp = inet6_sk(newsk);
 		newtp = tcp_sk(newsk);
 
+		memcpy(newnp, np, sizeof(struct ipv6_pinfo));
+
 		ipv6_addr_set(&newnp->daddr, 0, 0, htonl(0x0000FFFF),
 			      newinet->daddr);
 
@@ -1339,6 +1341,7 @@
 	newtp = tcp_sk(newsk);
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
+	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
 	ipv6_addr_copy(&newnp->daddr, &req->af.v6_req.rmt_addr);
 	ipv6_addr_copy(&newnp->saddr, &req->af.v6_req.loc_addr);
 	ipv6_addr_copy(&newnp->rcv_saddr, &req->af.v6_req.loc_addr);

Re: [BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Arnaldo Carvalho de Melo]

Em Tue, Jun 18, 2002 at 12:58:04AM -0300, Arnaldo C. Melo escreveu:
> Em Mon, Jun 17, 2002 at 11:49:34PM -0300, Arnaldo C. Melo escreveu:
> > Em Mon, Jun 17, 2002 at 07:17:26PM -0700, David S. Miller escreveu:
> > >    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
> > >    Date: Mon, 17 Jun 2002 21:57:35 -0300
> > > 
> > >    --- orig/net/ipv6/tcp_ipv6.c	Sat May 25 23:13:56 2002
> > >    +++ linux/net/ipv6/tcp_ipv6.c	Fri Jun 14 23:23:07 2002
> > > 
> > > I've installed this change into my tree in the meantime.
> > > If we find a better fix, we can just revert this.
> > 
> > OK, I found a better fix, I think, that allowed me to kill inet6_sk_generic
> > in af_inet6.c, using a constructor for the tcpv6, udpv6 and raw6 slab caches,
> > as suggested by Russel King, I'll be sending RSN.
> 
> Here it is, David, please consider pulling it from:
> 
> http://kernel-acme.bkbits.net:8080/tcpv6-pinet6
> 
> Best Regards,
> 
> - Arnaldo

Oops, brain fart, David, please don't apply, in tcp_create_openreq_child
we overwrite the ->pinet6 field after the constructor inits it to the proper
value :( Please leave the old patch in place for a while... :(

- Arnaldo

Re: [BKPATCH] Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Tue, 18 Jun 2002 01:15:39 -0300
   
   Oops, brain fart, David, please don't apply, in tcp_create_openreq_child
   we overwrite the ->pinet6 field after the constructor inits it to the proper
   value :( Please leave the old patch in place for a while... :(

No problem.

Re: [PATCH][2.5.22] OOPS in tcp_v6_get_port

[Carl Ritson]

On Mon, 17 Jun 2002, Arnaldo Carvalho de Melo wrote:

> Em Mon, Jun 17, 2002 at 02:33:19PM -0700, David S. Miller escreveu:
> > 
> > This is a known bug introduced by the struct sock splitup into
> > external per-protocol pieces done by Arnaldo de Melo.  He is working
> > on the proper fix, your proposed change will just paper over the real
> > bug.

I suspected something like that.

> Carl,
> 
> 	Can you try this patch?
> 
> - Arnaldo
> 
> --- orig/net/ipv6/tcp_ipv6.c    Sat May 25 23:13:56 2002
> +++ linux/net/ipv6/tcp_ipv6.c   Fri Jun 14 23:23:07 2002
> <diff snipped> 

This patch doesn't help, it produces exactly the same oops when decoded, I 
assume the newer patch you mention to Dave is the correct fix?

Thanks,
Carl