[PATCH] Conntrack leak with raw sockets

[PATCH] Conntrack leak with raw sockets

[Phil Oester]

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

In the event a raw socket is created for sending purposes only, the creator
never bothers to check the socket's receive queue.  But we continue to
add skbs to its queue until it fills up.

Unfortunately, if ip_conntrack is loaded on the box, each skb we add to the
queue potentially holds a reference to a conntrack.  If the user attempts
to unload ip_conntrack, we will spin around forever since the queued skbs
are pinned.

This behaviour can be witnessed in Fedora distributions which use
NetworkManager.  Arguably there should be an option to create a 'sending-only'
socket which won't suffer from this problem, but in the interim I think
the cleanest solution is to drop the conntrack reference before adding the skb
to the socket's queue.  The below patch does just that.

This fixes Netfilter bugzilla #91 and Redhat bugzilla #112630.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>




[-- Attachment #2: patch-ref --]
[-- Type: text/plain, Size: 727 bytes --]

diff -ru linux-orig/net/packet/af_packet.c linux-new/net/packet/af_packet.c
--- linux-orig/net/packet/af_packet.c	2005-03-25 14:48:13.597903552 -0500
+++ linux-new/net/packet/af_packet.c	2005-03-25 14:51:05.480773384 -0500
@@ -274,6 +274,10 @@
 	dst_release(skb->dst);
 	skb->dst = NULL;
 
+	/* drop conntrack reference */
+	nf_conntrack_put(skb->nfct);
+	skb->nfct = NULL;
+
 	spkt = (struct sockaddr_pkt*)skb->cb;
 
 	skb_push(skb, skb->data-skb->mac.raw);
@@ -517,6 +521,10 @@
 	dst_release(skb->dst);
 	skb->dst = NULL;
 
+	/* drop conntrack reference */
+	nf_conntrack_put(skb->nfct);
+	skb->nfct = NULL;
+
 	spin_lock(&sk->sk_receive_queue.lock);
 	po->stats.tp_packets++;
 	__skb_queue_tail(&sk->sk_receive_queue, skb);

Re: [PATCH] Conntrack leak with raw sockets

[Patrick McHardy]

Phil Oester wrote:
> In the event a raw socket is created for sending purposes only, the creator
> never bothers to check the socket's receive queue.  But we continue to
> add skbs to its queue until it fills up.
> 
> Unfortunately, if ip_conntrack is loaded on the box, each skb we add to the
> queue potentially holds a reference to a conntrack.  If the user attempts
> to unload ip_conntrack, we will spin around forever since the queued skbs
> are pinned.

Great work tracking this down. But I fear the problem will come back
haunt us with this patch. The are more places where a packet can be
queued indefinitely, for example stopped qdiscs. IMO the best fix
is to drop the conntrack reference once the packet leaves IP, so we
don't have to make any assumptions about what will happen to the
packet - this would be in ip_finish_output2(). Could you send a patch
that does this? While you're at it, you could also remove this part
from ip_conntrack_standalone:

#if !defined(CONFIG_IP_NF_NAT) && !defined(CONFIG_IP_NF_NAT_MODULE)
         /* Previously seen (loopback)?  Ignore.  Do this before
            fragment check. */
         if ((*pskb)->nfct)
                 return NF_ACCEPT;
#endif

Regards
Patrick

Re: [PATCH] Conntrack leak with raw sockets

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
> 
> Great work tracking this down. But I fear the problem will come back
> haunt us with this patch. The are more places where a packet can be
> queued indefinitely, for example stopped qdiscs. IMO the best fix
> is to drop the conntrack reference once the packet leaves IP, so we
> don't have to make any assumptions about what will happen to the
> packet - this would be in ip_finish_output2(). Could you send a patch
> that does this? While you're at it, you could also remove this part
> from ip_conntrack_standalone:

Agreed.

BTW, please use nf_reset() instead of open coding this.

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

Re: [PATCH] Conntrack leak with raw sockets

[Phil Oester]

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

On Fri, Mar 25, 2005 at 09:37:01PM +0100, Patrick McHardy wrote:
> Great work tracking this down. But I fear the problem will come back
> haunt us with this patch. The are more places where a packet can be
> queued indefinitely, for example stopped qdiscs. IMO the best fix
> is to drop the conntrack reference once the packet leaves IP, so we
> don't have to make any assumptions about what will happen to the
> packet - this would be in ip_finish_output2(). Could you send a patch

Something like the attached?

Phil



[-- Attachment #2: patch-ref2 --]
[-- Type: text/plain, Size: 1111 bytes --]

diff -ru linux-orig/net/ipv4/ip_output.c linux-new/net/ipv4/ip_output.c
--- linux-orig/net/ipv4/ip_output.c	2005-03-25 14:48:13.543911760 -0500
+++ linux-new/net/ipv4/ip_output.c	2005-03-26 01:01:13.064616240 -0500
@@ -195,6 +195,8 @@
 	nf_debug_ip_finish_output2(skb);
 #endif /*CONFIG_NETFILTER_DEBUG*/
 
+	nf_reset(skb);
+
 	if (hh) {
 		int hh_alen;
 
diff -ru linux-orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux-new/net/ipv4/netfilter/ip_conntrack_standalone.c
--- linux-orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-03-25 14:48:13.550910696 -0500
+++ linux-new/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-03-26 01:03:09.916852000 -0500
@@ -423,13 +423,6 @@
 				        const struct net_device *out,
 				        int (*okfn)(struct sk_buff *))
 {
-#if !defined(CONFIG_IP_NF_NAT) && !defined(CONFIG_IP_NF_NAT_MODULE)
-	/* Previously seen (loopback)?  Ignore.  Do this before
-           fragment check. */
-	if ((*pskb)->nfct)
-		return NF_ACCEPT;
-#endif
-
 	/* Gather fragments. */
 	if ((*pskb)->nh.iph->frag_off & htons(IP_MF|IP_OFFSET)) {
 		*pskb = ip_ct_gather_frags(*pskb,

Re: [PATCH] Conntrack leak with raw sockets

[Patrick McHardy]

Phil Oester wrote:
> Something like the attached?

Perfect, thanks. I'll push it to Dave with the next batch of netfilter
patches.

Regards
Patrick