[PATCH] fix dst_entry leak in icmp_push_reply()

[PATCH] fix dst_entry leak in icmp_push_reply()

[Ollie Wild]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

If the ip_append_data() call in icmp_push_reply() fails, 
ip_flush_pending_frames() needs to be called.  Otherwise, ip_rt_put() is 
never called on inet_sk(icmp_socket->sk)->cork.rt, which prevents the 
route (and net_device) from ever being freed.

I've attached a patch which fixes the problem.

Ollie Wild

[-- Attachment #2: icmp_push_reply.patch --]
[-- Type: text/x-patch, Size: 334 bytes --]

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -368,6 +368,8 @@ static void icmp_push_reply(struct icmp_
 		icmph->checksum = csum_fold(csum);
 		skb->ip_summed = CHECKSUM_NONE;
 		ip_push_pending_frames(icmp_socket->sk);
+	} else {
+		ip_flush_pending_frames(icmp_socket->sk);
 	}
 }
 

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Patrick McHardy]

Ollie Wild wrote:
> If the ip_append_data() call in icmp_push_reply() fails, 
> ip_flush_pending_frames() needs to be called.  Otherwise, ip_rt_put() is 
> never called on inet_sk(icmp_socket->sk)->cork.rt, which prevents the 
> route (and net_device) from ever being freed.
> 
> I've attached a patch which fixes the problem.
> 
> Ollie Wild
> 
> 
> ------------------------------------------------------------------------
> 
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -368,6 +368,8 @@ static void icmp_push_reply(struct icmp_
>  		icmph->checksum = csum_fold(csum);
>  		skb->ip_summed = CHECKSUM_NONE;
>  		ip_push_pending_frames(icmp_socket->sk);
> +	} else {
> +		ip_flush_pending_frames(icmp_socket->sk);
>

Your patch doesn't fit your description, the else-condition you're
adding triggers when the queue is empty, so what is the point?

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Ollie Wild]

Patrick McHardy wrote:

> Ollie Wild wrote:
>
>> If the ip_append_data() call in icmp_push_reply() fails, 
>> ip_flush_pending_frames() needs to be called.  Otherwise, ip_rt_put() 
>> is never called on inet_sk(icmp_socket->sk)->cork.rt, which prevents 
>> the route (and net_device) from ever being freed.
>
>
> Your patch doesn't fit your description, the else-condition you're
> adding triggers when the queue is empty, so what is the point?

Since we're only calling ip_append_data() once here, the two conditions 
are identical.

Ollie

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Patrick McHardy]

Ollie Wild wrote:
> Patrick McHardy wrote:
> 
>> Ollie Wild wrote:
>>
>>> If the ip_append_data() call in icmp_push_reply() fails,
>>> ip_flush_pending_frames() needs to be called.  Otherwise, ip_rt_put()
>>> is never called on inet_sk(icmp_socket->sk)->cork.rt, which prevents
>>> the route (and net_device) from ever being freed.
>>
>> Your patch doesn't fit your description, the else-condition you're
>> adding triggers when the queue is empty, so what is the point?
> 
> Since we're only calling ip_append_data() once here, the two conditions
> are identical.

You're right, I misread your patch. It would be easier to understand
if you just checked the return value of ip_append_data, as done in
udp.c or raw.c.

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Ollie Wild]

Ollie Wild wrote:

> Patrick McHardy wrote:
>
>> Your patch doesn't fit your description, the else-condition you're
>> adding triggers when the queue is empty, so what is the point?
>
>
> Since we're only calling ip_append_data() once here, the two 
> conditions are identical.

I should mention that this problem is not academic.  We've run into it 
in the field.  If a lot of ICMP destination unreachable messages are 
generated (by flooding a net_device with bad UDP packets for instance), 
the net_device can no longer be unregistered.

That said, I appreciate that the if-else condition doesn't seem quite 
right.  The problem is, the icmp_push_reply() routine is implicitly 
using the queue as a success indicator.  I put the 
ip_flush_pending_frames() call inside the else block because I wanted to 
guarantee that one of ip_push_pending_frames() and 
ip_flush_pending_frames() is always called.  Both will do proper cleanup.

I'm open to suggestions if you think there's a cleaner way to implement 
this.

Ollie

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

Ollie Wild wrote:
> That said, I appreciate that the if-else condition doesn't seem quite
> right.  The problem is, the icmp_push_reply() routine is implicitly
> using the queue as a success indicator.  I put the
> ip_flush_pending_frames() call inside the else block because I wanted to
> guarantee that one of ip_push_pending_frames() and
> ip_flush_pending_frames() is always called.  Both will do proper cleanup.
> 
> I'm open to suggestions if you think there's a cleaner way to implement
> this.

Checking the return value of ip_append_data seems cleaner to me.
Patch attached.

Signed-off-by: Patrick McHardy <kaber@trash.net>

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 844 bytes --]

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -349,12 +349,12 @@ static void icmp_push_reply(struct icmp_
 {
 	struct sk_buff *skb;
 
-	ip_append_data(icmp_socket->sk, icmp_glue_bits, icmp_param,
-		       icmp_param->data_len+icmp_param->head_len,
-		       icmp_param->head_len,
-		       ipc, rt, MSG_DONTWAIT);
-
-	if ((skb = skb_peek(&icmp_socket->sk->sk_write_queue)) != NULL) {
+	if (ip_append_data(icmp_socket->sk, icmp_glue_bits, icmp_param,
+		           icmp_param->data_len+icmp_param->head_len,
+		           icmp_param->head_len,
+		           ipc, rt, MSG_DONTWAIT) < 0)
+		ip_flush_pending_frames(icmp_socket->sk);
+	else if ((skb = skb_peek(&icmp_socket->sk->sk_write_queue)) != NULL) {
 		struct icmphdr *icmph = skb->h.icmph;
 		unsigned int csum = 0;
 		struct sk_buff *skb1;

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[Ollie Wild]

Patrick McHardy wrote:

>Checking the return value of ip_append_data seems cleaner to me.
>Patch attached.
>  
>
Works for me.

Thanks,
Ollie

Re: [PATCH] fix dst_entry leak in icmp_push_reply()

[David S. Miller]

From: Ollie Wild <aaw@rincewind.tv>
Date: Thu, 18 Aug 2005 12:05:31 -0700

> Patrick McHardy wrote:
> 
> >Checking the return value of ip_append_data seems cleaner to me.
> >Patch attached.
> >  
> >
> Works for me.

Applied, thanks everyone.