[PATCH][IPV6] fixed authentication error with TCP

[PATCH][IPV6] fixed authentication error with TCP

[Kazunori Miyazawa]

Hello,

Miss Joy (@IBM) and I investigated the bug that "authentication error" occured with
using TCP and AH in IPv6. This patch fixes the bug. This patch makes the kernel consider
extension header length in a dst.

This pach works with my previous patch which fixes zero-clear in ah6_input.

Please append the name "Joy Latten" into the log.

#I'm in summer holidays until 10th August. I will response very slowly because I only have
 dial-up line with 30kbps :-p

Best regards,

diff -ruN a/include/net/ipv6.h b/include/net/ipv6.h
--- a/include/net/ipv6.h	2003-07-28 02:07:24.000000000 +0900
+++ b/include/net/ipv6.h	2003-08-06 14:10:36.000000000 +0900
@@ -353,9 +353,7 @@
 
 extern void			ip6_flush_pending_frames(struct sock *sk);
 
-extern int			ip6_dst_lookup(struct sock *sk,
-					       struct dst_entry **dst,
-					       struct flowi *fl);
+extern struct dst_entry *	ip6_dst_lookup(struct sock *sk, struct flowi *fl);
 
 /*
  *	skb processing functions
diff -ruN a/net/ipv6/icmp.c b/net/ipv6/icmp.c
--- a/net/ipv6/icmp.c	2003-07-28 01:59:40.000000000 +0900
+++ b/net/ipv6/icmp.c	2003-08-06 14:20:29.000000000 +0900
@@ -355,8 +355,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	err = ip6_dst_lookup(sk, &dst, &fl);
-	if (err) goto out;
+	dst = ip6_dst_lookup(sk, &fl);
+	if (dst->error) goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
@@ -434,9 +434,9 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	err = ip6_dst_lookup(sk, &dst, &fl);
+	dst = ip6_dst_lookup(sk, &fl);
 
-	if (err) goto out;
+	if (dst->error) goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
diff -ruN a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
--- a/net/ipv6/ip6_output.c	2003-07-28 01:57:01.000000000 +0900
+++ b/net/ipv6/ip6_output.c	2003-08-06 15:35:23.000000000 +0900
@@ -211,10 +211,6 @@
 	u32 mtu;
 	int err = 0;
 
-	if ((err = xfrm_lookup(&skb->dst, fl, sk, 0)) < 0) {
-		return err;
-	}
-
 	if (opt) {
 		int head_room;
 
@@ -1141,72 +1137,73 @@
 	return err;
 }
 
-int ip6_dst_lookup(struct sock *sk, struct dst_entry **dst, struct flowi *fl)
+struct dst_entry *ip6_dst_lookup(struct sock *sk, struct flowi *fl)
 {
-	struct ipv6_pinfo *np = inet6_sk(sk);
+	struct dst_entry *dst = NULL;
 	int err = 0;
 
-	*dst = __sk_dst_check(sk, np->dst_cookie);
-	if (*dst) {
-		struct rt6_info *rt = (struct rt6_info*)*dst;
-
-			/* Yes, checking route validity in not connected
-			   case is not very simple. Take into account,
-			   that we do not support routing by source, TOS,
-			   and MSG_DONTROUTE 		--ANK (980726)
-
-			   1. If route was host route, check that
-			      cached destination is current.
-			      If it is network route, we still may
-			      check its validity using saved pointer
-			      to the last used address: daddr_cache.
-			      We do not want to save whole address now,
-			      (because main consumer of this service
-			       is tcp, which has not this problem),
-			      so that the last trick works only on connected
-			      sockets.
-			   2. oif also should be the same.
-			 */
-
-		if (((rt->rt6i_dst.plen != 128 ||
-		      ipv6_addr_cmp(&fl->fl6_dst, &rt->rt6i_dst.addr))
-		     && (np->daddr_cache == NULL ||
-			 ipv6_addr_cmp(&fl->fl6_dst, np->daddr_cache)))
-		    || (fl->oif && fl->oif != (*dst)->dev->ifindex)) {
-			*dst = NULL;
-		} else
-			dst_hold(*dst);
+	if (sk) {
+		struct ipv6_pinfo *np = inet6_sk(sk);
+	
+		dst = __sk_dst_check(sk, np->dst_cookie);
+		if (dst) {
+			struct rt6_info *rt = (struct rt6_info*)dst;
+	
+				/* Yes, checking route validity in not connected
+				   case is not very simple. Take into account,
+				   that we do not support routing by source, TOS,
+				   and MSG_DONTROUTE 		--ANK (980726)
+	
+				   1. If route was host route, check that
+				      cached destination is current.
+				      If it is network route, we still may
+				      check its validity using saved pointer
+				      to the last used address: daddr_cache.
+				      We do not want to save whole address now,
+				      (because main consumer of this service
+				       is tcp, which has not this problem),
+				      so that the last trick works only on connected
+				      sockets.
+				   2. oif also should be the same.
+				 */
+	
+			if (((rt->rt6i_dst.plen != 128 ||
+			      ipv6_addr_cmp(&fl->fl6_dst, &rt->rt6i_dst.addr))
+			     && (np->daddr_cache == NULL ||
+				 ipv6_addr_cmp(&fl->fl6_dst, np->daddr_cache)))
+			    || (fl->oif && fl->oif != dst->dev->ifindex)) {
+				dst = NULL;
+			} else
+				dst_hold(dst);
+		}
 	}
 
-	if (*dst == NULL)
-		*dst = ip6_route_output(sk, fl);
+	if (dst == NULL)
+		dst = ip6_route_output(sk, fl);
 
-	if ((*dst)->error) {
-		IP6_INC_STATS(Ip6OutNoRoutes);
-		dst_release(*dst);
-		return -ENETUNREACH;
-	}
+	if (dst->error)
+		return dst;
 
 	if (ipv6_addr_any(&fl->fl6_src)) {
-		err = ipv6_get_saddr(*dst, &fl->fl6_dst, &fl->fl6_src);
+		err = ipv6_get_saddr(dst, &fl->fl6_dst, &fl->fl6_src);
 
 		if (err) {
 #if IP6_DEBUG >= 2
 			printk(KERN_DEBUG "ip6_build_xmit: "
 			       "no available source address\n");
 #endif
-			return err;
+			dst->error = err;
+			return dst;
 		}
 	}
 
-        if (*dst) {
-		if ((err = xfrm_lookup(dst, fl, sk, 0)) < 0) {
-			dst_release(*dst);	
-			return -ENETUNREACH;
+        if (dst) {
+		if ((err = xfrm_lookup(&dst, fl, sk, 0)) < 0) {
+			dst->error = -ENETUNREACH;
 		}
         }
 
-	return 0;
+	return dst;
 }
 
 int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, int offset, int len, int odd, struct sk_buff *skb),
diff -ruN a/net/ipv6/raw.c b/net/ipv6/raw.c
--- a/net/ipv6/raw.c	2003-07-28 02:00:40.000000000 +0900
+++ b/net/ipv6/raw.c	2003-08-06 14:19:32.000000000 +0900
@@ -658,8 +658,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	err = ip6_dst_lookup(sk, &dst, &fl);
-	if (err)
+	dst = ip6_dst_lookup(sk, &fl);
+	if (dst->error)
 		goto out;
 
 	if (hlimit < 0) {
diff -ruN a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
--- a/net/ipv6/tcp_ipv6.c	2003-07-28 02:03:09.000000000 +0900
+++ b/net/ipv6/tcp_ipv6.c	2003-08-06 16:13:21.000000000 +0900
@@ -663,7 +663,7 @@
 		ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 	}
 
-	dst = ip6_route_output(sk, &fl);
+	dst = ip6_dst_lookup(sk, &fl);
 
 	if ((err = dst->error) != 0) {
 		dst_release(dst);
@@ -691,6 +691,8 @@
 	tp->ext_header_len = 0;
 	if (np->opt)
 		tp->ext_header_len = np->opt->opt_flen + np->opt->opt_nflen;
+	tp->ext2_header_len = dst->header_len;
+
 	tp->mss_clamp = IPV6_MIN_MTU - sizeof(struct tcphdr) - sizeof(struct ipv6hdr);
 
 	inet->dport = usin->sin6_port;
@@ -788,7 +790,7 @@
 			fl.fl_ip_dport = inet->dport;
 			fl.fl_ip_sport = inet->sport;
 
-			dst = ip6_route_output(sk, &fl);
+			dst = ip6_dst_lookup(sk, &fl);
 		} else
 			dst_hold(dst);
 
@@ -889,7 +891,7 @@
 			ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 		}
 
-		dst = ip6_route_output(sk, &fl);
+		dst = ip6_dst_lookup(sk, &fl);
 		if (dst->error)
 			goto done;
 	}
@@ -1018,7 +1020,7 @@
 	fl.fl_ip_sport = t1->source;
 
 	/* sk = NULL, but it is safe for now. RST socket required. */
-	buff->dst = ip6_route_output(NULL, &fl);
+	buff->dst = ip6_dst_lookup(NULL, &fl);
 
 	if (buff->dst->error == 0) {
 		ip6_xmit(NULL, buff, &fl, NULL, 0);
@@ -1081,7 +1083,7 @@
 	fl.fl_ip_dport = t1->dest;
 	fl.fl_ip_sport = t1->source;
 
-	buff->dst = ip6_route_output(NULL, &fl);
+	buff->dst = ip6_dst_lookup(NULL, &fl);
 
 	if (buff->dst->error == 0) {
 		ip6_xmit(NULL, buff, &fl, NULL, 0);
@@ -1329,7 +1331,7 @@
 		fl.fl_ip_dport = req->rmt_port;
 		fl.fl_ip_sport = inet_sk(sk)->sport;
 
-		dst = ip6_route_output(sk, &fl);
+		dst = ip6_dst_lookup(sk, &fl);
 	}
 
 	if (dst->error)
@@ -1401,6 +1403,7 @@
 	if (newnp->opt)
 		newtp->ext_header_len = newnp->opt->opt_nflen +
 					newnp->opt->opt_flen;
+	newtp->ext2_header_len = dst->header_len;
 
 	tcp_sync_mss(newsk, dst_pmtu(dst));
 	newtp->advmss = dst_metric(dst, RTAX_ADVMSS);
@@ -1727,7 +1730,7 @@
 			ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 		}
 
-		dst = ip6_route_output(sk, &fl);
+		dst = ip6_dst_lookup(sk, &fl);
 
 		if (dst->error) {
 			err = dst->error;
@@ -1770,7 +1773,7 @@
 	dst = __sk_dst_check(sk, np->dst_cookie);
 
 	if (dst == NULL) {
-		dst = ip6_route_output(sk, &fl);
+		dst = ip6_dst_lookup(sk, &fl);
 
 		if (dst->error) {
 			sk->sk_err_soft = -dst->error;
diff -ruN a/net/ipv6/udp.c b/net/ipv6/udp.c
--- a/net/ipv6/udp.c	2003-07-28 02:07:29.000000000 +0900
+++ b/net/ipv6/udp.c	2003-08-06 14:19:23.000000000 +0900
@@ -928,8 +928,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	err = ip6_dst_lookup(sk, &dst, &fl);
-	if (err)
+	dst = ip6_dst_lookup(sk, &fl);
+	if (dst->error)
 		goto out;
 
 	if (hlimit < 0) {

Re: [PATCH][IPV6] fixed authentication error with TCP

[David S. Miller]

On Wed, 6 Aug 2003 16:44:13 +0900
Kazunori Miyazawa <kazunori@miyazawa.org> wrote:

> Miss Joy (@IBM) and I investigated the bug that "authentication
> error" occured with using TCP and AH in IPv6. This patch fixes the
> bug. This patch makes the kernel consider extension header length in
> a dst.
> 
> This pach works with my previous patch which fixes zero-clear in ah6_input.
> 
> Please append the name "Joy Latten" into the log.

I have applied this patch, thank you.

But I see a small area for improvement.  Look at the place inside
of ip6_dst_lookup() where we do source address selection.  If this
fails, we mark error to dst->error.

Is it correct?  This 'dst' route might otherwise be perfectly fine.
But now that dst->error is set, it is poisoned for other users
and they are not able to use it.

A similar case occurs further down after the xfrm_lookup() call, but
this one I think is correct.

It seems to me that it is only OK for dst->error to be set on routes
that may not be used validly for anything.

Alexey, do I understand this stuff correctly?

Re: [PATCH][IPV6] fixed authentication error with TCP

[kuznet]

Hello!

> But I see a small area for improvement.  Look at the place inside
> of ip6_dst_lookup() where we do source address selection.  If this
> fails, we mark error to dst->error.
.....
> It seems to me that it is only OK for dst->error to be set on routes
> that may not be used validly for anything.
> 
> Alexey, do I understand this stuff correctly?

I think you do. And this is rather severe bug than area for improvement.
It definitely corrupts routing table.

Well, I think switching from function returning error code to a function
returning dst is not a very good idea. This never was convenient.
In the case of error, IPv6 used to return to caller a dummy reject route,
which is always -ENETRUNREACH. So, to do this we have to hold a route
for each errno. Returning int was just better.

Alexey

Re: [PATCH][IPV6] fixed authentication error with TCP

[David S. Miller]

On Mon, 18 Aug 2003 10:45:41 +0300 (EEST)
Ville Nuorvala <vnuorval@tcs.hut.fi> wrote:

> The attached patch reverts to the old ip6_dst_lookup() interface and and
> makes tcp_ipv6.c use that instead.
> 
> As an added bonus neither tcp_v6_connect() nor udpv6_connect() needs to do
> source address selection anymore, since ip6_dst_lookup() already does this
> for them.

Thanks a lot for doing this work, I'll review and apply
your patch.

I was about to code this up myself :)

Re: [PATCH][IPV6] fixed authentication error with TCP

[Ville Nuorvala]

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

On Mon, 18 Aug 2003 kuznet@ms2.inr.ac.ru wrote:

> Hello!
>
> > But I see a small area for improvement.  Look at the place inside
> > of ip6_dst_lookup() where we do source address selection.  If this
> > fails, we mark error to dst->error.
> .....
> > It seems to me that it is only OK for dst->error to be set on routes
> > that may not be used validly for anything.
> >
> > Alexey, do I understand this stuff correctly?
>
> I think you do. And this is rather severe bug than area for improvement.
> It definitely corrupts routing table.

Besides this, the patch also introduced dst_entry leaks in at least
icmp.c, raw.c and udp.c.
>
> Well, I think switching from function returning error code to a function
> returning dst is not a very good idea. This never was convenient.
> In the case of error, IPv6 used to return to caller a dummy reject route,
> which is always -ENETRUNREACH. So, to do this we have to hold a route
> for each errno. Returning int was just better.

The attached patch reverts to the old ip6_dst_lookup() interface and and
makes tcp_ipv6.c use that instead.

As an added bonus neither tcp_v6_connect() nor udpv6_connect() needs to do
source address selection anymore, since ip6_dst_lookup() already does this
for them.

>
> Alexey

I've tested the patch a bit and everything seems to work normally, so its
probably safe to apply it :)

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: 8608 bytes --]

diff -Nur --exclude=RCS --exclude=CVS --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/include/net/ipv6.h linux-2.5/include/net/ipv6.h
--- linux-2.5.OLD/include/net/ipv6.h	Mon Aug 18 10:04:07 2003
+++ linux-2.5/include/net/ipv6.h	Fri Aug 15 13:29:00 2003
@@ -353,7 +353,9 @@
 
 extern void			ip6_flush_pending_frames(struct sock *sk);
 
-extern struct dst_entry *	ip6_dst_lookup(struct sock *sk, struct flowi *fl);
+extern int			ip6_dst_lookup(struct sock *sk,
+					       struct dst_entry **dst,
+					       struct flowi *fl);
 
 /*
  *	skb processing functions
diff -Nur --exclude=RCS --exclude=CVS --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/net/ipv6/icmp.c linux-2.5/net/ipv6/icmp.c
--- linux-2.5.OLD/net/ipv6/icmp.c	Mon Aug 18 10:04:23 2003
+++ linux-2.5/net/ipv6/icmp.c	Fri Aug 15 13:29:03 2003
@@ -355,8 +355,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	dst = ip6_dst_lookup(sk, &fl);
-	if (dst->error) goto out;
+	err = ip6_dst_lookup(sk, &dst, &fl);
+	if (err) goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
@@ -434,9 +434,9 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	dst = ip6_dst_lookup(sk, &fl);
+	err = ip6_dst_lookup(sk, &dst, &fl);
 
-	if (dst->error) goto out;
+	if (err) goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
diff -Nur --exclude=RCS --exclude=CVS --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/net/ipv6/ip6_output.c linux-2.5/net/ipv6/ip6_output.c
--- linux-2.5.OLD/net/ipv6/ip6_output.c	Mon Aug 18 10:04:23 2003
+++ linux-2.5/net/ipv6/ip6_output.c	Fri Aug 15 13:51:11 2003
@@ -1136,17 +1136,16 @@
 	return err;
 }
 
-struct dst_entry *ip6_dst_lookup(struct sock *sk, struct flowi *fl)
+int ip6_dst_lookup(struct sock *sk, struct dst_entry **dst, struct flowi *fl)
 {
-	struct dst_entry *dst = NULL;
 	int err = 0;
 
 	if (sk) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 	
-		dst = __sk_dst_check(sk, np->dst_cookie);
-		if (dst) {
-			struct rt6_info *rt = (struct rt6_info*)dst;
+		*dst = __sk_dst_check(sk, np->dst_cookie);
+		if (*dst) {
+			struct rt6_info *rt = (struct rt6_info*)*dst;
 	
 				/* Yes, checking route validity in not connected
 				   case is not very simple. Take into account,
@@ -1170,39 +1169,38 @@
 			      ipv6_addr_cmp(&fl->fl6_dst, &rt->rt6i_dst.addr))
 			     && (np->daddr_cache == NULL ||
 				 ipv6_addr_cmp(&fl->fl6_dst, np->daddr_cache)))
-			    || (fl->oif && fl->oif != dst->dev->ifindex)) {
-				dst = NULL;
+			    || (fl->oif && fl->oif != (*dst)->dev->ifindex)) {
+				*dst = NULL;
 			} else
-				dst_hold(dst);
+				dst_hold(*dst);
 		}
 	}
 
-	if (dst == NULL)
-		dst = ip6_route_output(sk, fl);
-
-	if (dst->error)
-		return dst;
+	if (*dst == NULL)
+		*dst = ip6_route_output(sk, fl);
 
+	if ((err = (*dst)->error)) {
+		dst_release(*dst);
+		return err;
+	}
 	if (ipv6_addr_any(&fl->fl6_src)) {
-		err = ipv6_get_saddr(dst, &fl->fl6_dst, &fl->fl6_src);
+		err = ipv6_get_saddr(*dst, &fl->fl6_dst, &fl->fl6_src);
 
 		if (err) {
 #if IP6_DEBUG >= 2
-			printk(KERN_DEBUG "ip6_build_xmit: "
+			printk(KERN_DEBUG "ip6_dst_lookup: "
 			       "no available source address\n");
 #endif
-			dst->error = err;
-			return dst;
+			dst_release(*dst);
+			return err;
 		}
 	}
-
-        if (dst) {
-		if ((err = xfrm_lookup(&dst, fl, sk, 0)) < 0) {
-			dst->error = -ENETUNREACH;
-		}
+	if ((err = xfrm_lookup(dst, fl, sk, 0)) < 0) {
+		dst_release(*dst);
+		return -ENETUNREACH;
         }
 
-	return dst;
+	return 0;
 }
 
 int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, int offset, int len, int odd, struct sk_buff *skb),
diff -Nur --exclude=RCS --exclude=CVS --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/net/ipv6/raw.c linux-2.5/net/ipv6/raw.c
--- linux-2.5.OLD/net/ipv6/raw.c	Mon Aug 18 10:04:24 2003
+++ linux-2.5/net/ipv6/raw.c	Fri Aug 15 13:33:14 2003
@@ -658,8 +658,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	dst = ip6_dst_lookup(sk, &fl);
-	if ((err = dst->error))
+	err = ip6_dst_lookup(sk, &dst, &fl);
+	if (err)
 		goto out;
 
 	if (hlimit < 0) {
diff -Nur --exclude=RCS --exclude=CVS --exclude=SCCS --exclude=BitKeeper --exclude=ChangeSet linux-2.5.OLD/net/ipv6/tcp_ipv6.c linux-2.5/net/ipv6/tcp_ipv6.c
--- linux-2.5.OLD/net/ipv6/tcp_ipv6.c	Mon Aug 18 10:04:24 2003
+++ linux-2.5/net/ipv6/tcp_ipv6.c	Fri Aug 15 17:24:06 2003
@@ -663,19 +663,12 @@
 		ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 	}
 
-	dst = ip6_dst_lookup(sk, &fl);
+	err = ip6_dst_lookup(sk, &dst, &fl);
 
-	if ((err = dst->error) != 0) {
-		dst_release(dst);
+	if (err)
 		goto failure;
-	}
 
 	if (saddr == NULL) {
-		err = ipv6_get_saddr(dst, &np->daddr, &fl.fl6_src);
-		if (err) {
-			dst_release(dst);
-			goto failure;
-		}
 		saddr = &fl.fl6_src;
 		ipv6_addr_copy(&np->rcv_saddr, saddr);
 	}
@@ -790,13 +783,14 @@
 			fl.fl_ip_dport = inet->dport;
 			fl.fl_ip_sport = inet->sport;
 
-			dst = ip6_dst_lookup(sk, &fl);
+			if ((err = ip6_dst_lookup(sk, &dst, &fl))) {
+				sk->sk_err_soft = -err;
+				goto out;
+			}
 		} else
 			dst_hold(dst);
 
-		if (dst->error) {
-			sk->sk_err_soft = -dst->error;
-		} else if (tp->pmtu_cookie > dst_pmtu(dst)) {
+		if (tp->pmtu_cookie > dst_pmtu(dst)) {
 			tcp_sync_mss(sk, dst_pmtu(dst));
 			tcp_simple_retransmit(sk);
 		} /* else let the usual retransmit timer handle it */
@@ -891,8 +885,8 @@
 			ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 		}
 
-		dst = ip6_dst_lookup(sk, &fl);
-		if (dst->error)
+		err = ip6_dst_lookup(sk, &dst, &fl);
+		if (err)
 			goto done;
 	}
 
@@ -1020,9 +1014,7 @@
 	fl.fl_ip_sport = t1->source;
 
 	/* sk = NULL, but it is safe for now. RST socket required. */
-	buff->dst = ip6_dst_lookup(NULL, &fl);
-
-	if (buff->dst->error == 0) {
+	if (!ip6_dst_lookup(NULL, &buff->dst, &fl)) {
 		ip6_xmit(NULL, buff, &fl, NULL, 0);
 		TCP_INC_STATS_BH(TcpOutSegs);
 		TCP_INC_STATS_BH(TcpOutRsts);
@@ -1083,9 +1075,7 @@
 	fl.fl_ip_dport = t1->dest;
 	fl.fl_ip_sport = t1->source;
 
-	buff->dst = ip6_dst_lookup(NULL, &fl);
-
-	if (buff->dst->error == 0) {
+	if (!ip6_dst_lookup(NULL, &buff->dst, &fl)) {
 		ip6_xmit(NULL, buff, &fl, NULL, 0);
 		TCP_INC_STATS_BH(TcpOutSegs);
 		return;
@@ -1331,11 +1321,9 @@
 		fl.fl_ip_dport = req->rmt_port;
 		fl.fl_ip_sport = inet_sk(sk)->sport;
 
-		dst = ip6_dst_lookup(sk, &fl);
-	}
-
-	if (dst->error)
-		goto out;
+		if (ip6_dst_lookup(sk, &dst, &fl))
+			goto out;
+	} 
 
 	newsk = tcp_create_openreq_child(sk, req, skb);
 	if (newsk == NULL)
@@ -1730,11 +1718,9 @@
 			ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 		}
 
-		dst = ip6_dst_lookup(sk, &fl);
+		err = ip6_dst_lookup(sk, &dst, &fl);
 
-		if (dst->error) {
-			err = dst->error;
-			dst_release(dst);
+		if (err) {
 			sk->sk_route_caps = 0;
 			return err;
 		}
@@ -1773,12 +1759,11 @@
 	dst = __sk_dst_check(sk, np->dst_cookie);
 
 	if (dst == NULL) {
-		dst = ip6_dst_lookup(sk, &fl);
+		int err = ip6_dst_lookup(sk, &dst, &fl);
 
-		if (dst->error) {
-			sk->sk_err_soft = -dst->error;
-			dst_release(dst);
-			return -sk->sk_err_soft;
+		if (err) {
+			sk->sk_err_soft = -err;
+			return err;
 		}
 
 		ip6_dst_store(sk, dst, NULL);
diff -Nur --exclude=RCS --exclude=CVS --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 Aug 18 10:04:24 2003
+++ linux-2.5/net/ipv6/udp.c	Fri Aug 15 13:34:28 2003
@@ -330,21 +330,11 @@
 		ipv6_addr_copy(&fl.fl6_dst, rt0->addr);
 	}
 
-	dst = ip6_route_output(sk, &fl);
-
-	if ((err = dst->error) != 0) {
-		dst_release(dst);
+	err = ip6_dst_lookup(sk, &dst, &fl);
+	if (err)
 		goto out;
-	}
-
-	/* get the source address used in the appropriate device */
 
-	err = ipv6_get_saddr(dst, daddr, &fl.fl6_src);
-
-	if (err) {
-		dst_release(dst);
-		goto out;
-	}
+	/* source address lookup done in ip6_dst_lookup */
 
 	if (ipv6_addr_any(&np->saddr))
 		ipv6_addr_copy(&np->saddr, &fl.fl6_src);
@@ -930,8 +920,8 @@
 	if (!fl.oif && ipv6_addr_is_multicast(&fl.fl6_dst))
 		fl.oif = np->mcast_oif;
 
-	dst = ip6_dst_lookup(sk, &fl);
-	if ((err = dst->error))
+	err = ip6_dst_lookup(sk, &dst, &fl);
+	if (err)
 		goto out;
 
 	if (hlimit < 0) {

Re: [PATCH][IPV6] fixed authentication error with TCP

[Kazunori Miyazawa]

I don't stick to chenge the interface.
I think his/her patch is better than mine. I checked and it seems to work well.
Please apply his/her patch.

On Mon, 18 Aug 2003 10:45:41 +0300 (EEST)
Ville Nuorvala <vnuorval@tcs.hut.fi> wrote:

> On Mon, 18 Aug 2003 kuznet@ms2.inr.ac.ru wrote:
> 
> > Hello!
> >
> > > But I see a small area for improvement.  Look at the place inside
> > > of ip6_dst_lookup() where we do source address selection.  If this
> > > fails, we mark error to dst->error.
> > .....
> > > It seems to me that it is only OK for dst->error to be set on routes
> > > that may not be used validly for anything.
> > >
> > > Alexey, do I understand this stuff correctly?
> >
> > I think you do. And this is rather severe bug than area for improvement.
> > It definitely corrupts routing table.
> 
> Besides this, the patch also introduced dst_entry leaks in at least
> icmp.c, raw.c and udp.c.
> >
> > Well, I think switching from function returning error code to a function
> > returning dst is not a very good idea. This never was convenient.
> > In the case of error, IPv6 used to return to caller a dummy reject route,
> > which is always -ENETRUNREACH. So, to do this we have to hold a route
> > for each errno. Returning int was just better.
> 
> The attached patch reverts to the old ip6_dst_lookup() interface and and
> makes tcp_ipv6.c use that instead.
> 
> As an added bonus neither tcp_v6_connect() nor udpv6_connect() needs to do
> source address selection anymore, since ip6_dst_lookup() already does this
> for them.
> 
> >
> > Alexey
> 
> I've tested the patch a bit and everything seems to work normally, so its
> probably safe to apply it :)
> 
> 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
> 

--Kazunori Miyazawa

Re: [PATCH][IPV6] fixed authentication error with TCP

[David S. Miller]

On Mon, 18 Aug 2003 10:45:41 +0300 (EEST)
Ville Nuorvala <vnuorval@tcs.hut.fi> wrote:

> The attached patch reverts to the old ip6_dst_lookup() interface and and
> makes tcp_ipv6.c use that instead.
> 
> As an added bonus neither tcp_v6_connect() nor udpv6_connect() needs to do
> source address selection anymore, since ip6_dst_lookup() already does this
> for them.

While verifying this patch, I discovered some new dst leaks.

For example:

1) In icmpv6_send(), who releases the DST?
2) Similarly, for icmpv6_echo_reply()?

In these two cases, ip6_append_data() grabs one reference
each time it attaches 'rt' to the np->cort.rt, but we still
have the singular reference in those two icmpv6_*() routines
referenced above and they leak.

3) ip6_push_pending_frames(), it gets a new reference to
   np->cork.rt to attach the 'dst' to skb->dst on output.
   Then it sets np->cork.rt to NULL, 1 reference is lost
   as a result.
4) Similarly in ip6_flush_pending_frames().

I don't want to check any more places, because every place where I
look in ipv6 I find a new DST leak :(

Re: [PATCH][IPV6] fixed authentication error with TCP

[David S. Miller]

On Mon, 18 Aug 2003 10:45:41 +0300 (EEST)
Ville Nuorvala <vnuorval@tcs.hut.fi> wrote:

> The attached patch reverts to the old ip6_dst_lookup() interface and and
> makes tcp_ipv6.c use that instead.

I applied your patch with a small change, I made it so
that *dst is set to NULL in every case that ip6_dst_lookup()
returned an error.

In particular, there were some error paths still in tcp_ipv6.c
that would dst_release() in the error path which only works if
dst is set to NULL in ip6_dst_lookup() error cases.

Re: [PATCH][IPV6] fixed authentication error with TCP

[David S. Miller]

On Mon, 18 Aug 2003 02:48:37 -0700
"David S. Miller" <davem@redhat.com> wrote:

> While verifying this patch, I discovered some new dst leaks.

This patch fixes them up.

# 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.1209  -> 1.1210 
#	net/ipv6/ip6_output.c	1.42    -> 1.43   
#	     net/ipv6/icmp.c	1.39    -> 1.40   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/08/18	davem@nuts.ninka.net	1.1210
# [IPV6]: Fix some dst cache leaks.
# 1) icmpv6_send() and icmpv6_echo_reply() never release dst.
# 2) ip6_{push,flush}_pending_frames() leak np->cork.rt.
# --------------------------------------------
#
diff -Nru a/net/ipv6/icmp.c b/net/ipv6/icmp.c
--- a/net/ipv6/icmp.c	Mon Aug 18 03:39:55 2003
+++ b/net/ipv6/icmp.c	Mon Aug 18 03:39:55 2003
@@ -356,7 +356,8 @@
 		fl.oif = np->mcast_oif;
 
 	err = ip6_dst_lookup(sk, &dst, &fl);
-	if (err) goto out;
+	if (err)
+		goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
@@ -375,7 +376,7 @@
 		if (net_ratelimit())
 			printk(KERN_DEBUG "icmp: len problem\n");
 		__skb_push(skb, plen);
-		goto out;
+		goto out_dst_release;
 	}
 
 	idev = in6_dev_get(skb->dev);
@@ -396,6 +397,8 @@
 out_put:
 	if (likely(idev != NULL))
 		in6_dev_put(idev);
+out_dst_release:
+	dst_release(dst);
 out:
 	icmpv6_xmit_unlock();
 }
@@ -435,8 +438,8 @@
 		fl.oif = np->mcast_oif;
 
 	err = ip6_dst_lookup(sk, &dst, &fl);
-
-	if (err) goto out;
+	if (err)
+		goto out;
 
 	if (hlimit < 0) {
 		if (ipv6_addr_is_multicast(&fl.fl6_dst))
@@ -465,6 +468,7 @@
 out_put: 
 	if (likely(idev != NULL))
 		in6_dev_put(idev);
+	dst_release(dst);
 out: 
 	icmpv6_xmit_unlock();
 }
diff -Nru a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
--- a/net/ipv6/ip6_output.c	Mon Aug 18 03:39:55 2003
+++ b/net/ipv6/ip6_output.c	Mon Aug 18 03:39:55 2003
@@ -1484,6 +1484,7 @@
 		np->cork.opt = NULL;
 	}
 	if (np->cork.rt) {
+		dst_release(&np->cork.rt->u.dst);
 		np->cork.rt = NULL;
 	}
 	if (np->cork.fl) {
@@ -1510,6 +1511,7 @@
 		np->cork.opt = NULL;
 	}
 	if (np->cork.rt) {
+		dst_release(&np->cork.rt->u.dst);
 		np->cork.rt = NULL;
 	}
 	if (np->cork.fl) {