[PATCH] IPV6: Sereral errors on udpv6_connect()

[PATCH] IPV6: Sereral errors on udpv6_connect()

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

The CONFIG_IPV6_SUBTREE contains multiple fixes and changes.
I'm trying to split them.

This patch fixes multiple errors in udpv6_connect().
 - pointer within an automatic storage class variable fl was illegally cached
   using ip6_dst_store().
 - uninitialized saddr was copied to fl.fl6_src.
 - don't cache if ipv6_saddr_get() failed.
Patch is based on CONFIG_IPV6_SUBTREE patch from Ville Nuorvala 
<vnuorval@tcs.hut.fi>.

Index: linux25-LINUS/net/ipv6/udp.c
===================================================================
RCS file: /cvsroot/usagi/usagi-backport/linux25/net/ipv6/udp.c,v
retrieving revision 1.1.1.18
diff -u -r1.1.1.18 udp.c
--- linux25-LINUS/net/ipv6/udp.c	26 May 2003 08:04:11 -0000	1.1.1.18
+++ linux25-LINUS/net/ipv6/udp.c	4 Jun 2003 00:29:32 -0000
@@ -254,7 +254,6 @@
 	struct inet_opt      	*inet = inet_sk(sk);
 	struct ipv6_pinfo      	*np = inet6_sk(sk);
 	struct in6_addr		*daddr;
-	struct in6_addr		saddr;
 	struct dst_entry	*dst;
 	struct flowi		fl;
 	struct ip6_flowlabel	*flowlabel = NULL;
@@ -355,7 +354,7 @@
 
 	fl.proto = IPPROTO_UDP;
 	ipv6_addr_copy(&fl.fl6_dst, &np->daddr);
-	ipv6_addr_copy(&fl.fl6_src, &saddr);
+	ipv6_addr_copy(&fl.fl6_src, &np->saddr);
 	fl.oif = sk->bound_dev_if;
 	fl.fl_ip_dport = inet->dport;
 	fl.fl_ip_sport = inet->sport;
@@ -381,20 +380,23 @@
 		return err;
 	}
 
-	ip6_dst_store(sk, dst, &fl.fl6_dst);
-
 	/* get the source address used in the appropriate device */
 
-	err = ipv6_get_saddr(dst, daddr, &saddr);
+	err = ipv6_get_saddr(dst, daddr, &fl.fl6_src);
 
 	if (err == 0) {
 		if (ipv6_addr_any(&np->saddr))
-			ipv6_addr_copy(&np->saddr, &saddr);
+			ipv6_addr_copy(&np->saddr, &fl.fl6_src);
 
 		if (ipv6_addr_any(&np->rcv_saddr)) {
-			ipv6_addr_copy(&np->rcv_saddr, &saddr);
+			ipv6_addr_copy(&np->rcv_saddr, &fl.fl6_src);
 			inet->rcv_saddr = LOOPBACK4_IPV6;
 		}
+
+		ip6_dst_store(sk, dst,
+			      !ipv6_addr_cmp(&fl.fl6_dst, &np->daddr) ?
+			      &np->daddr : NULL);
+
 		sk->state = TCP_ESTABLISHED;
 	}
 	fl6_sock_release(flowlabel);

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] IPV6: Sereral errors on udpv6_connect()

[David S. Miller]

   From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
   Date: Wed, 04 Jun 2003 09:39:44 +0900 (JST)
   
   This patch fixes multiple errors in udpv6_connect().
    - pointer within an automatic storage class variable fl was illegally cached
      using ip6_dst_store().
    - uninitialized saddr was copied to fl.fl6_src.
    - don't cache if ipv6_saddr_get() failed.

Applied.

All these kinds of things need to be done differently once
routing by saddr is supported, more specifically when
route6 lookups make source address selection.  Look at
ipv4 side to see the kind of thing I'm talking about.

Yoshfuji-san, remember when Alexey wanted you to change your
source address selection so that it occurred at routing layer?
This is exactly what I'm talking about.

In my view, ipv6 routing is merely a SEVERELY crippled version
of ipv4 routing.  Most of ipv6 routing changes needed amount
to merely "porting over" existing ipv4 routing features.

[patch] IPV6: Refcount leaks in udpv6_connect()

[Ville Nuorvala]

Hi!

A dst refcount leak had unfortunately crept into my original
CONFIG_IPV6_SUBTREES patch and your derived udpv6_connect() patch
(changeset 1.1215.68.12).

While fixing this, I also found and fixed some other apparent leaks.

This patch fixes several bugs in udpv6_connect():
- dst refcount leak if ipv6_get_saddr() fails
- several flowlabel refcount leaks

The diff is done against ChangeSet 1.1307.

Thanks,
Ville

diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5/net/ipv6/udp.c merge-2.5/net/ipv6/udp.c
--- linux-2.5/net/ipv6/udp.c	Fri Jun 13 16:08:00 2003
+++ merge-2.5/net/ipv6/udp.c	Fri Jun 13 16:05:33 2003
@@ -299,9 +299,10 @@
 	if (addr_type == IPV6_ADDR_MAPPED) {
 		struct sockaddr_in sin;

-		if (__ipv6_only_sock(sk))
-			return -ENETUNREACH;
-
+		if (__ipv6_only_sock(sk)) {
+			err = -ENETUNREACH;
+			goto out;
+		}
 		sin.sin_family = AF_INET;
 		sin.sin_addr.s_addr = daddr->s6_addr32[3];
 		sin.sin_port = usin->sin6_port;
@@ -309,8 +310,8 @@
 		err = udp_connect(sk, (struct sockaddr*) &sin, sizeof(sin));

 ipv4_connected:
-		if (err < 0)
-			return err;
+		if (err)
+			goto out;

 		ipv6_addr_set(&np->daddr, 0, 0, htonl(0x0000ffff), inet->daddr);

@@ -323,7 +324,7 @@
 			ipv6_addr_set(&np->rcv_saddr, 0, 0, htonl(0x0000ffff),
 				      inet->rcv_saddr);
 		}
-		return 0;
+		goto out;
 	}

 	if (addr_type&IPV6_ADDR_LINKLOCAL) {
@@ -331,8 +332,8 @@
 		    usin->sin6_scope_id) {
 			if (sk->sk_bound_dev_if &&
 			    sk->sk_bound_dev_if != usin->sin6_scope_id) {
-				fl6_sock_release(flowlabel);
-				return -EINVAL;
+				err = -EINVAL;
+				goto out;
 			}
 			sk->sk_bound_dev_if = usin->sin6_scope_id;
 			if (!sk->sk_bound_dev_if &&
@@ -341,8 +342,10 @@
 		}

 		/* Connect to link-local address requires an interface */
-		if (!sk->sk_bound_dev_if)
-			return -EINVAL;
+		if (!sk->sk_bound_dev_if) {
+			err = -EINVAL;
+			goto out;
+		}
 	}

 	ipv6_addr_copy(&np->daddr, daddr);
@@ -379,31 +382,33 @@

 	if ((err = dst->error) != 0) {
 		dst_release(dst);
-		fl6_sock_release(flowlabel);
-		return err;
+		goto out;
 	}

 	/* get the source address used in the appropriate device */

 	err = ipv6_get_saddr(dst, daddr, &fl.fl6_src);

-	if (err == 0) {
-		if (ipv6_addr_any(&np->saddr))
-			ipv6_addr_copy(&np->saddr, &fl.fl6_src);
-
-		if (ipv6_addr_any(&np->rcv_saddr)) {
-			ipv6_addr_copy(&np->rcv_saddr, &fl.fl6_src);
-			inet->rcv_saddr = LOOPBACK4_IPV6;
-		}
+	if (err) {
+		dst_release(dst);
+		goto out;
+	}

-		ip6_dst_store(sk, dst,
-			      !ipv6_addr_cmp(&fl.fl6_dst, &np->daddr) ?
-			      &np->daddr : NULL);
+	if (ipv6_addr_any(&np->saddr))
+		ipv6_addr_copy(&np->saddr, &fl.fl6_src);

-		sk->sk_state = TCP_ESTABLISHED;
+	if (ipv6_addr_any(&np->rcv_saddr)) {
+		ipv6_addr_copy(&np->rcv_saddr, &fl.fl6_src);
+		inet->rcv_saddr = LOOPBACK4_IPV6;
 	}
-	fl6_sock_release(flowlabel);

+	ip6_dst_store(sk, dst,
+		      !ipv6_addr_cmp(&fl.fl6_dst, &np->daddr) ?
+		      &np->daddr : NULL);
+
+	sk->sk_state = TCP_ESTABLISHED;
+out:
+	fl6_sock_release(flowlabel);
 	return err;
 }

--
Ville Nuorvala
Research Assistant, Institute of Digital Communications,
Helsinki University of Technology
email: vnuorval@tcs.hut.fi, phone: +358 (0)9 451 5257

Re: [patch] IPV6: Refcount leaks in udpv6_connect()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <Pine.LNX.4.44.0306132149290.4922-100000@rhea.tcs.hut.fi> (at Sat, 14 Jun 2003 00:07:25 +0300 (EEST)), Ville Nuorvala <vnuorval@tcs.hut.fi> says:

> A dst refcount leak had unfortunately crept into my original
> CONFIG_IPV6_SUBTREES patch and your derived udpv6_connect() patch
> (changeset 1.1215.68.12).

patch looks fine to me.

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [patch] IPV6: Refcount leaks in udpv6_connect()

[David S. Miller]

   From: Ville Nuorvala <vnuorval@tcs.hut.fi>
   Date: Sat, 14 Jun 2003 00:07:25 +0300 (EEST)
   
   The diff is done against ChangeSet 1.1307.

Your patch does not apply without rejects.
Please regenerate.

Re: [patch] IPV6: Refcount leaks in udpv6_connect()

[Ville Nuorvala]

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

On Sun, 15 Jun 2003, David S. Miller wrote:

> Your patch does not apply without rejects.
> Please regenerate.

Hmm, odd. Perhaps the inlining of the patch mangled it somehow. Anyhow,
here's a bk treediff against CS 1.1318 as an attachment.

Hope this applies :)

Thanks,
Ville
--
Ville Nuorvala
Research Assistant, Institute of Digital Communications,
Helsinki University of Technology
email: vnuorval@tcs.hut.fi, phone: +358 (0)9 451 5257

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

diff -Nur --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/net/ipv6/udp.c linux-2.5/net/ipv6/udp.c
--- linux-2.5.OLD/net/ipv6/udp.c	Mon Jun 16 12:02:08 2003
+++ linux-2.5/net/ipv6/udp.c	Sun Jun 15 23:10:03 2003
@@ -299,9 +299,10 @@
 	if (addr_type == IPV6_ADDR_MAPPED) {
 		struct sockaddr_in sin;
 
-		if (__ipv6_only_sock(sk))
-			return -ENETUNREACH;
-
+		if (__ipv6_only_sock(sk)) {
+			err = -ENETUNREACH;
+			goto out;
+		}
 		sin.sin_family = AF_INET;
 		sin.sin_addr.s_addr = daddr->s6_addr32[3];
 		sin.sin_port = usin->sin6_port;
@@ -309,8 +310,8 @@
 		err = udp_connect(sk, (struct sockaddr*) &sin, sizeof(sin));
 
 ipv4_connected:
-		if (err < 0)
-			return err;
+		if (err)
+			goto out;
 		
 		ipv6_addr_set(&np->daddr, 0, 0, htonl(0x0000ffff), inet->daddr);
 
@@ -323,7 +324,7 @@
 			ipv6_addr_set(&np->rcv_saddr, 0, 0, htonl(0x0000ffff),
 				      inet->rcv_saddr);
 		}
-		return 0;
+		goto out;
 	}
 
 	if (addr_type&IPV6_ADDR_LINKLOCAL) {
@@ -331,8 +332,8 @@
 		    usin->sin6_scope_id) {
 			if (sk->sk_bound_dev_if &&
 			    sk->sk_bound_dev_if != usin->sin6_scope_id) {
-				fl6_sock_release(flowlabel);
-				return -EINVAL;
+				err = -EINVAL;
+				goto out;
 			}
 			sk->sk_bound_dev_if = usin->sin6_scope_id;
 			if (!sk->sk_bound_dev_if &&
@@ -341,8 +342,10 @@
 		}
 
 		/* Connect to link-local address requires an interface */
-		if (!sk->sk_bound_dev_if)
-			return -EINVAL;
+		if (!sk->sk_bound_dev_if) {
+			err = -EINVAL;
+			goto out;
+		}
 	}
 
 	ipv6_addr_copy(&np->daddr, daddr);
@@ -379,31 +382,33 @@
 
 	if ((err = dst->error) != 0) {
 		dst_release(dst);
-		fl6_sock_release(flowlabel);
-		return err;
+		goto out;
 	}
 
 	/* get the source address used in the appropriate device */
 
 	err = ipv6_get_saddr(dst, daddr, &fl.fl6_src);
 
-	if (err == 0) {
-		if (ipv6_addr_any(&np->saddr))
-			ipv6_addr_copy(&np->saddr, &fl.fl6_src);
-
-		if (ipv6_addr_any(&np->rcv_saddr)) {
-			ipv6_addr_copy(&np->rcv_saddr, &fl.fl6_src);
-			inet->rcv_saddr = LOOPBACK4_IPV6;
-		}
+	if (err) {
+		dst_release(dst);
+		goto out;
+	}
 
-		ip6_dst_store(sk, dst,
-			      !ipv6_addr_cmp(&fl.fl6_dst, &np->daddr) ?
-			      &np->daddr : NULL);
+	if (ipv6_addr_any(&np->saddr))
+		ipv6_addr_copy(&np->saddr, &fl.fl6_src);
 
-		sk->sk_state = TCP_ESTABLISHED;
+	if (ipv6_addr_any(&np->rcv_saddr)) {
+		ipv6_addr_copy(&np->rcv_saddr, &fl.fl6_src);
+		inet->rcv_saddr = LOOPBACK4_IPV6;
 	}
-	fl6_sock_release(flowlabel);
 
+	ip6_dst_store(sk, dst,
+		      !ipv6_addr_cmp(&fl.fl6_dst, &np->daddr) ?
+		      &np->daddr : NULL);
+
+	sk->sk_state = TCP_ESTABLISHED;
+out:
+	fl6_sock_release(flowlabel);
 	return err;
 }
 

Re: [patch] IPV6: Refcount leaks in udpv6_connect()

[David S. Miller]

   From: Ville Nuorvala <vnuorval@tcs.hut.fi>
   Date: Mon, 16 Jun 2003 12:22:57 +0300 (EEST)
   
   Hmm, odd. Perhaps the inlining of the patch mangled it somehow. Anyhow,
   here's a bk treediff against CS 1.1318 as an attachment.

Thanks, I've applied your fix.