* [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code.
@ 2008-03-25 15:40 Pavel Emelyanov
2008-03-26 3:32 ` Herbert Xu
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Emelyanov @ 2008-03-25 15:40 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Denis Lunev, Linux Netdev List, devel
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 <xemul@openvz.org>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index ff9a8e6..db231cb 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -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);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code.
2008-03-25 15:40 [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code Pavel Emelyanov
@ 2008-03-26 3:32 ` Herbert Xu
2008-03-26 7:46 ` Denis V. Lunev
2008-03-26 9:02 ` Pavel Emelyanov
0 siblings, 2 replies; 5+ messages in thread
From: Herbert Xu @ 2008-03-26 3:32 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, Denis Lunev, Linux Netdev List, devel
On Tue, Mar 25, 2008 at 06:40:00PM +0300, Pavel Emelyanov wrote:
> 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 <xemul@openvz.org>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
Thanks for catching this!
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index ff9a8e6..db231cb 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -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.
> 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;
These ones look good.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code.
2008-03-26 3:32 ` Herbert Xu
@ 2008-03-26 7:46 ` Denis V. Lunev
2008-03-26 9:02 ` Pavel Emelyanov
1 sibling, 0 replies; 5+ messages in thread
From: Denis V. Lunev @ 2008-03-26 7:46 UTC (permalink / raw)
To: Herbert Xu; +Cc: Pavel Emelyanov, David Miller, Linux Netdev List, devel
On Wed, 2008-03-26 at 11:32 +0800, Herbert Xu wrote:
> On Tue, Mar 25, 2008 at 06:40:00PM +0300, Pavel Emelyanov wrote:
> > 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 <xemul@openvz.org>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
>
> Thanks for catching this!
>
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > index ff9a8e6..db231cb 100644
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code.
2008-03-26 3:32 ` Herbert Xu
2008-03-26 7:46 ` Denis V. Lunev
@ 2008-03-26 9:02 ` Pavel Emelyanov
2008-03-26 9:06 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Pavel Emelyanov @ 2008-03-26 9:02 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, Denis Lunev, Linux Netdev List, devel
Herbert Xu wrote:
> On Tue, Mar 25, 2008 at 06:40:00PM +0300, Pavel Emelyanov wrote:
>> 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 <xemul@openvz.org>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>
> Thanks for catching this!
>
>> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>> index ff9a8e6..db231cb 100644
>> --- a/net/ipv4/icmp.c
>> +++ b/net/ipv4/icmp.c
>> @@ -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.
>> 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;
>
> These ones look good.
Thanks :)
> Cheers,
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-03-26 9:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-25 15:40 [PATCH][ICMP]: Dst entry leak in icmp_send host re-lookup code Pavel Emelyanov
2008-03-26 3:32 ` Herbert Xu
2008-03-26 7:46 ` Denis V. Lunev
2008-03-26 9:02 ` Pavel Emelyanov
2008-03-26 9:06 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).