fix dst_entry leak in icmp_push_reply()

PATCH Applied Ollie Wild 3 participants 12 messages Aug 17, 2005 – Aug 18, 2005

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Ollie Wild

author

2005-08-17 20:21 UTC #0 lore raw

PATCH 1/1 fix dst_entry leak in icmp_push_reply()

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

net/ipv4/icmp.c +2 -0

@@ -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);
 	}
 }

Patrick McHardy

2005-08-17 23:57 UTC #1 lore raw

Quoted 13 lines · Ollie Wild

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
------------------------------------------------------------------------

net/ipv4/icmp.c static void icmp_push_reply(struct icmp_ · line 373

Quoted 6 lines

net/ipv4/icmp.c +2 -0

@@ -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?

Ollie Wild

author

2005-08-18 06:41 UTC #2 lore raw

Quoted 7 lines · Patrick McHardy

Quoted 1 line · Ollie Wild
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

Patrick McHardy

2005-08-18 18:42 UTC #3 lore raw

Quoted 9 lines · Ollie Wild

Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Ollie Wild
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.

Quoted 5 lines

Quoted 2 lines
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.

Patrick McHardy

2005-08-18 18:42 UTC #4 lore raw

In reply to Ollie Wild

Quoted 9 lines · Ollie Wild

Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Ollie Wild
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.

Quoted 5 lines

Quoted 2 lines
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.

Ollie Wild

author

2005-08-18 18:45 UTC #5 lore raw

In reply to Ollie Wild

Quoted 7 lines · Ollie Wild

Quoted 2 lines · Patrick McHardy
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

Patrick McHardy

2005-08-18 18:59 UTC #6 lore raw

Quoted 9 lines · Ollie Wild

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 <[email protected]>

net/ipv4/icmp.c +6 -6

@@ -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;

Patrick McHardy

2005-08-18 18:59 UTC #7 lore raw

In reply to Ollie Wild

Quoted 9 lines · Ollie Wild

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 <[email protected]>

net/ipv4/icmp.c +6 -6

@@ -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;