Skip to contentSkip to conversation

: Dst entry leak in icmp_send host re-lookup code.

PATCH In review Pavel Emelyanov 4 participants 5 messages Mar 25, 2008 – Mar 26, 2008

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 1/1 : Dst entry leak in icmp_send host re-lookup code.

Commit 8b7817f3a959ed99d7443afc12f78a7e1fcc2063 ([IPSEC]: Add ICMP host relookup support) introduced some dst leaks on error paths: the rt pointer can be forgotten to be put. Fix it bu going to a proper label.

Found after net namespace's lo refused to unregister :) Many thanks to Den for valuable help during debugging.

Signed-off-by Pavel Emelyanov <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>

---

net/ipv4/icmp.c +4 -4
index ff9a8e6..db231cb 100644
@@ -594,11 +594,11 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 			rt = NULL;
 			break;
 		default:
-			goto out_unlock;
+			goto ende;
 		}
 
 		if (xfrm_decode_session_reverse(skb_in, &fl, AF_INET))
-			goto out_unlock;
+			goto ende;
 
 		if (inet_addr_type(net, fl.fl4_src) == RTN_LOCAL)
 			err = __ip_route_output_key(net, &rt2, &fl);

@@ -608,7 +608,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 
 			fl2.fl4_dst = fl.fl4_src;
 			if (ip_route_output_key(net, &rt2, &fl2))
-				goto out_unlock;
+				goto ende;
 
 			/* Ugh! */
 			odst = skb_in->dst;

@@ -621,7 +621,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 
 		if (err)
-			goto out_unlock;
+			goto ende;
 
 		err = xfrm_lookup((struct dst_entry **)&rt2, &fl, NULL,
 				  XFRM_LOOKUP_ICMP);
#1 lore raw
Quoted 9 lines · Pavel Emelyanov

Commit 8b7817f3a959ed99d7443afc12f78a7e1fcc2063 ([IPSEC]: Add ICMP host relookup support) introduced some dst leaks on error paths: the rt pointer can be forgotten to be put. Fix it bu going to a proper label.

Found after net namespace's lo refused to unregister :) Many thanks to Den for valuable help during debugging.

Signed-off-by Pavel Emelyanov <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>

Thanks for catching this!

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 598

Quoted 11 lines
net/ipv4/icmp.c +1 -1
index ff9a8e6..db231cb 100644
@@ -594,11 +594,11 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 			rt = NULL;
 			break;
 		default:
-			goto out_unlock;
+			goto ende;
 		}

I'm not sure about this bit though because xfrm_lookup is meant to free the route on error.

Quoted 16 lines
net/ipv4/icmp.c +2 -2
 		if (xfrm_decode_session_reverse(skb_in, &fl, AF_INET))
-			goto out_unlock;
+			goto ende;
 
 		if (inet_addr_type(net, fl.fl4_src) == RTN_LOCAL)
 			err = __ip_route_output_key(net, &rt2, &fl);

@@ -608,7 +608,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 
 			fl2.fl4_dst = fl.fl4_src;
 			if (ip_route_output_key(net, &rt2, &fl2))
-				goto out_unlock;
+				goto ende;
 
 			/* Ugh! */
 			odst = skb_in->dst;

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 624

Quoted 5 lines
net/ipv4/icmp.c +1 -1
@@ -621,7 +621,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 
 		if (err)
-			goto out_unlock;
+			goto ende;

These ones look good.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#2 lore raw
Quoted 13 lines · Herbert Xu
Quoted 9 lines · Pavel Emelyanov

Commit 8b7817f3a959ed99d7443afc12f78a7e1fcc2063 ([IPSEC]: Add ICMP host relookup support) introduced some dst leaks on error paths: the rt pointer can be forgotten to be put. Fix it bu going to a proper label.

Found after net namespace's lo refused to unregister :) Many thanks to Den for valuable help during debugging.

Signed-off-by Pavel Emelyanov <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>

Thanks for catching this!

Quoted 14 lines

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 598

Quoted 11 lines
net/ipv4/icmp.c +1 -1
index ff9a8e6..db231cb 100644
@@ -594,11 +594,11 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 			rt = NULL;
 			break;
 		default:
-			goto out_unlock;
+			goto ende;
 		}

I'm not sure about this bit though because xfrm_lookup is meant to free the route on error.

This is not a problem, it assigns NULL in this case to a pointer.

author
#3 lore raw
In reply to Herbert Xu
Quoted 13 lines · Herbert Xu
Quoted 9 lines · Pavel Emelyanov

Commit 8b7817f3a959ed99d7443afc12f78a7e1fcc2063 ([IPSEC]: Add ICMP host relookup support) introduced some dst leaks on error paths: the rt pointer can be forgotten to be put. Fix it bu going to a proper label.

Found after net namespace's lo refused to unregister :) Many thanks to Den for valuable help during debugging.

Signed-off-by Pavel Emelyanov <[email protected]>
Signed-off-by Denis V. Lunev <[email protected]>

Thanks for catching this!

Quoted 14 lines

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 598

Quoted 11 lines
net/ipv4/icmp.c +1 -1
index ff9a8e6..db231cb 100644
@@ -594,11 +594,11 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 			rt = NULL;
 			break;
 		default:
-			goto out_unlock;
+			goto ende;
 		}

I'm not sure about this bit though because xfrm_lookup is meant to free the route on error.

Well, it indeed does, we've missed that fact. Even though it is set to NULL, as Denis pointed out, this hunk is now needed here.

David, plz, disregard this patch, I'll send a new one in a moment.

Quoted 18 lines
Quoted 18 lines
net/ipv4/icmp.c +2 -2
 		if (xfrm_decode_session_reverse(skb_in, &fl, AF_INET))
-			goto out_unlock;
+			goto ende;
 
 		if (inet_addr_type(net, fl.fl4_src) == RTN_LOCAL)
 			err = __ip_route_output_key(net, &rt2, &fl);

@@ -608,7 +608,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 
 			fl2.fl4_dst = fl.fl4_src;
 			if (ip_route_output_key(net, &rt2, &fl2))
-				goto out_unlock;
+				goto ende;
 
 			/* Ugh! */
 			odst = skb_in->dst;

@@ -621,7 +621,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
Quoted 5 lines

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 624

Quoted 3 lines
net/ipv4/icmp.c +1 -1
 		if (err)
-			goto out_unlock;
+			goto ende;

These ones look good.

Thanks :)

Quoted 1 line

Cheers,

maintainer
#4 lore raw

From: Pavel Emelyanov <[email protected]>
Date: Wed, 26 Mar 2008 12:02:34 +0300

Quoted 1 line

David, plz, disregard this patch, I'll send a new one in a moment.

Ok, but please fix the net-2.6.26 build regression I just reported first.