Fix conntrack iteration.

PATCH In review Rusty Russell 2 participants 4 messages Dec 24, 2004 – Dec 28, 2004

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Rusty Russell

author

2004-12-24 11:35 UTC #0 lore raw

PATCH 1/1 Fix conntrack iteration.

Found this bug while searching for another. Potential crash on unload (unlikely though, unless packets are queued).

Name: Fix ip_ct_selective_cleanup(), and rename ip_ct_iterate_cleanup()
Status: Tested under nfsim

Signed-off-by Rusty Russell <[email protected]>

Several places use ip_ct_selective_cleanup() as a general iterator, which it was not intended for (it takes a const ip_conntrack *). So rename it, and make it take a non-const argument.

Also, it missed unconfirmed connections, which aren't in the hash table. This introduces a potential problem for users which expect to iterate all connections (such as the helper deletion code). So keep a linked list of unconfirmed connections as well.

net/ipv4/netfilter/ip_nat_core.c +3 -3

Index: linux-2.6.10-rc3-bk16-Netfilter/net/ipv4/netfilter/ip_nat_core.c
===================================================================
@@ -974,16 +974,16 @@
 }
 
 /* Clear NAT section of all conntracks, in case we're loaded again. */
-static int clean_nat(const struct ip_conntrack *i, void *data)
+static int clean_nat(struct ip_conntrack *i, void *data)
 {
-	memset((void *)&i->nat, 0, sizeof(i->nat));
+	memset(&i->nat, 0, sizeof(i->nat));
 	return 0;
 }
 
 /* Not __exit: called from ip_nat_standalone.c:init_or_cleanup() --RR */
 void ip_nat_cleanup(void)
 {
-	ip_ct_selective_cleanup(&clean_nat, NULL);
+	ip_ct_iterate_cleanup(&clean_nat, NULL);
 	ip_conntrack_destroyed = NULL;
 	vfree(bysource);
 }

net/ipv4/netfilter/ip_nat_helper.c +2 -2

Index: linux-2.6.10-rc3-bk16-Netfilter/net/ipv4/netfilter/ip_nat_helper.c
===================================================================
@@ -444,7 +444,7 @@
 }
 
 static int
-kill_helper(const struct ip_conntrack *i, void *helper)
+kill_helper(struct ip_conntrack *i, void *helper)
 {
 	int ret;

@@ -474,5 +474,5 @@
 	   forces admins to gen fake RSTs or bounce box, either of
 	   which is just a long-winded way of making things
 	   worse. --RR */
-	ip_ct_selective_cleanup(kill_helper, me);
+	ip_ct_iterate_cleanup(kill_helper, me);
 }

net/ipv4/netfilter/ip_conntrack_standalone.c +3 -3

Index: linux-2.6.10-rc3-bk16-Netfilter/net/ipv4/netfilter/ip_conntrack_standalone.c
===================================================================
@@ -48,7 +48,7 @@
 extern atomic_t ip_conntrack_count;
 DECLARE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
 
-static int kill_proto(const struct ip_conntrack *i, void *data)
+static int kill_proto(struct ip_conntrack *i, void *data)
 {
 	return (i->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum == 
 			*((u_int8_t *) data));

@@ -859,7 +859,7 @@
 	synchronize_net();
 
 	/* Remove all contrack entries for this protocol */
-	ip_ct_selective_cleanup(kill_proto, &proto->proto);
+	ip_ct_iterate_cleanup(kill_proto, &proto->proto);
 }
 
 static int __init init(void)

@@ -889,7 +889,7 @@
 EXPORT_SYMBOL(need_ip_conntrack);
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);
-EXPORT_SYMBOL(ip_ct_selective_cleanup);
+EXPORT_SYMBOL(ip_ct_iterate_cleanup);
 EXPORT_SYMBOL(ip_ct_refresh_acct);
 EXPORT_SYMBOL(ip_ct_protos);
 EXPORT_SYMBOL(ip_ct_find_proto);

include/linux/netfilter_ipv4/ip_conntrack.h +3 -3

Index: linux-2.6.10-rc3-bk16-Netfilter/include/linux/netfilter_ipv4/ip_conntrack.h
===================================================================
@@ -283,10 +283,10 @@
 struct sk_buff *
 ip_ct_gather_frags(struct sk_buff *skb);
 
-/* Delete all conntracks which match. */
+/* Iterate over all conntracks: if iter returns true, it's deleted. */
 extern void
-ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
-			void *data);
+ip_ct_iterate_cleanup(int (*iter)(struct ip_conntrack *i, void *data),
+		      void *data);
 
 /* It's confirmed if it is, or has been in the hash table. */
 static inline int is_confirmed(struct ip_conntrack *ct)

net/ipv4/netfilter/ipt_MASQUERADE.c +3 -3

Index: linux-2.6.10-rc3-bk16-Netfilter/net/ipv4/netfilter/ipt_MASQUERADE.c
===================================================================
@@ -118,7 +118,7 @@
 }
 
 static inline int
-device_cmp(const struct ip_conntrack *i, void *ifindex)
+device_cmp(struct ip_conntrack *i, void *ifindex)
 {
 	int ret;

@@ -141,7 +141,7 @@
 		   and forget them. */
 		IP_NF_ASSERT(dev->ifindex != 0);
 
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
+		ip_ct_iterate_cleanup(device_cmp, (void *)(long)dev->ifindex);
 	}
 
 	return NOTIFY_DONE;

@@ -159,7 +159,7 @@
 		   and forget them. */
 		IP_NF_ASSERT(dev->ifindex != 0);
 
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
+		ip_ct_iterate_cleanup(device_cmp, (void *)(long)dev->ifindex);
 	}
 
 	return NOTIFY_DONE;

net/ipv4/netfilter/ip_conntrack_core.c +35 ~~-15~~

127 lines not on this page · raw message

Signature · 1 line

A bad analogy is like a leaky screwdriver -- Richard Braakman

Patrick McHardy

2004-12-25 14:32 UTC #1 lore raw

Quoted 21 lines · Rusty Russell

Found this bug while searching for another. Potential crash on unload (unlikely though, unless packets are queued).
Name: Fix ip_ct_selective_cleanup(), and rename ip_ct_iterate_cleanup()
Status: Tested under nfsim
Signed-off-by Rusty Russell <[email protected]>
Several places use ip_ct_selective_cleanup() as a general iterator, which it was not intended for (it takes a const ip_conntrack *). So rename it, and make it take a non-const argument.
Also, it missed unconfirmed connections, which aren't in the hash table. This introduces a potential problem for users which expect to iterate all connections (such as the helper deletion code). So keep a linked list of unconfirmed connections as well.
Index: linux-2.6.10-rc3-bk16-Netfilter/net/ipv4/netfilter/ip_nat_core.c
===================================================================

net/ipv4/netfilter/ip_nat_core.c line 302

Quoted 7 lines

net/ipv4/netfilter/ip_nat_core.c +1 -3

@@ -298,9 +299,13 @@
 		ip_conntrack_destroyed(ct);
 
 	WRITE_LOCK(&ip_conntrack_lock);
-	/* Make sure don't leave any orphaned expectations lying around */
-	if (ct->expecting)
-		remove_expectations(ct, 1);
+	BUG_ON(ct->expecting);

This doesn't look right. The call to remove_expectations was added to deal with the TFTP helper registering expectations for unconfirmed connections.

Testcase from Phil Oester, who tracked down the problem:
--------
on a:
iptables -I INPUT -p udp --dport 69 -j DROP

on b:
tftp -c get a:foo

The expect is setup by the tftp helper, but the master conntrack is not
confirmed. Instead, the master is immediately sent to destroy_conntrack. Whoops! We've now got an orphaned expectation which will not go away (ever!).
--------

Regards
Patrick

Rusty Russell

author

2004-12-28 01:22 UTC #2 lore raw

Quoted 9 lines · Patrick McHardy

net/ipv4/netfilter/ip_conntrack_core.c line 302
Quoted 5 lines
net/ipv4/netfilter/ip_conntrack_core.c +1 -3
 	WRITE_LOCK(&ip_conntrack_lock);
-	/* Make sure don't leave any orphaned expectations lying around */
-	if (ct->expecting)
-		remove_expectations(ct, 1);
+	BUG_ON(ct->expecting);
This doesn't look right. The call to remove_expectations was added to deal with the TFTP helper registering expectations for unconfirmed connections.

Ah, right! I'll add a comment there instead. I couldn't see how it could happen, but this makes sense.

Thanks,
Rusty.

Signature · 1 line

A bad analogy is like a leaky screwdriver -- Richard Braakman

Rusty Russell

author

2004-12-28 01:45 UTC #3 lore raw

In reply to Patrick McHardy

Quoted 10 lines · Patrick McHardy

Review
Quoted 5 lines · Rusty Russell
(quoted diff) +1 -3
 	WRITE_LOCK(&ip_conntrack_lock);
-	/* Make sure don't leave any orphaned expectations lying around */
-	if (ct->expecting)
-		remove_expectations(ct, 1);
+	BUG_ON(ct->expecting);
This doesn't look right. The call to remove_expectations was added to deal with the TFTP helper registering expectations for unconfirmed connections.

Yep, good catch. Reverted that, added a comment in a separate patch.

Cheers,
Rusty.

Name: Fix ip_ct_selective_cleanup(), and rename ip_ct_iterate_cleanup()
Status: Tested under nfsim

Signed-off-by Rusty Russell <[email protected]>

Several places use ip_ct_selective_cleanup() as a general iterator, which it was not intended for (it takes a const ip_conntrack *). So rename it, and make it take a non-const argument.

net/ipv4/netfilter/ip_nat_core.c +3 -3

Index: linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ip_nat_core.c
===================================================================
@@ -1006,16 +1006,16 @@
 }
 
 /* Clear NAT section of all conntracks, in case we're loaded again. */
-static int clean_nat(const struct ip_conntrack *i, void *data)
+static int clean_nat(struct ip_conntrack *i, void *data)
 {
-	memset((void *)&i->nat, 0, sizeof(i->nat));
+	memset(&i->nat, 0, sizeof(i->nat));
 	return 0;
 }
 
 /* Not __exit: called from ip_nat_standalone.c:init_or_cleanup() --RR */
 void ip_nat_cleanup(void)
 {
-	ip_ct_selective_cleanup(&clean_nat, NULL);
+	ip_ct_iterate_cleanup(&clean_nat, NULL);
 	ip_conntrack_destroyed = NULL;
 	vfree(bysource);
 }

net/ipv4/netfilter/ip_conntrack_standalone.c +3 -3

Index: linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ip_conntrack_standalone.c
===================================================================
@@ -48,7 +48,7 @@
 extern atomic_t ip_conntrack_count;
 DECLARE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
 
-static int kill_proto(const struct ip_conntrack *i, void *data)
+static int kill_proto(struct ip_conntrack *i, void *data)
 {
 	return (i->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum == 
 			*((u_int8_t *) data));

@@ -859,7 +859,7 @@
 	synchronize_net();
 
 	/* Remove all contrack entries for this protocol */
-	ip_ct_selective_cleanup(kill_proto, &proto->proto);
+	ip_ct_iterate_cleanup(kill_proto, &proto->proto);
 }
 
 static int __init init(void)

@@ -890,7 +890,7 @@
 EXPORT_SYMBOL(need_ip_conntrack);
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);
-EXPORT_SYMBOL(ip_ct_selective_cleanup);
+EXPORT_SYMBOL(ip_ct_iterate_cleanup);
 EXPORT_SYMBOL(ip_ct_refresh_acct);
 EXPORT_SYMBOL(ip_ct_protos);
 EXPORT_SYMBOL(ip_ct_find_proto);

net/ipv4/netfilter/ip_nat_helper.c +2 -2

Index: linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ip_nat_helper.c
===================================================================
@@ -444,7 +444,7 @@
 }
 
 static int
-kill_helper(const struct ip_conntrack *i, void *helper)
+kill_helper(struct ip_conntrack *i, void *helper)
 {
 	int ret;

@@ -474,5 +474,5 @@
 	   forces admins to gen fake RSTs or bounce box, either of
 	   which is just a long-winded way of making things
 	   worse. --RR */
-	ip_ct_selective_cleanup(kill_helper, me);
+	ip_ct_iterate_cleanup(kill_helper, me);
 }

include/linux/netfilter_ipv4/ip_conntrack.h +3 -3

Index: linux-2.6.10-bk1-Netfilter/include/linux/netfilter_ipv4/ip_conntrack.h
===================================================================
@@ -283,10 +283,10 @@
 struct sk_buff *
 ip_ct_gather_frags(struct sk_buff *skb);
 
-/* Delete all conntracks which match. */
+/* Iterate over all conntracks: if iter returns true, it's deleted. */
 extern void
-ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
-			void *data);
+ip_ct_iterate_cleanup(int (*iter)(struct ip_conntrack *i, void *data),
+		      void *data);
 
 /* It's confirmed if it is, or has been in the hash table. */
 static inline int is_confirmed(struct ip_conntrack *ct)

net/ipv4/netfilter/ipt_MASQUERADE.c +3 -3

Index: linux-2.6.10-bk1-Netfilter/net/ipv4/netfilter/ipt_MASQUERADE.c
===================================================================
@@ -118,7 +118,7 @@
 }
 
 static inline int
-device_cmp(const struct ip_conntrack *i, void *ifindex)
+device_cmp(struct ip_conntrack *i, void *ifindex)
 {
 	int ret;

@@ -141,7 +141,7 @@
 		   and forget them. */
 		IP_NF_ASSERT(dev->ifindex != 0);
 
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
+		ip_ct_iterate_cleanup(device_cmp, (void *)(long)dev->ifindex);
 	}
 
 	return NOTIFY_DONE;

@@ -159,7 +159,7 @@
 		   and forget them. */
 		IP_NF_ASSERT(dev->ifindex != 0);
 
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
+		ip_ct_iterate_cleanup(device_cmp, (void *)(long)dev->ifindex);
 	}
 
 	return NOTIFY_DONE;

net/ipv4/netfilter/ip_conntrack_core.c +34 ~~-12~~

123 lines not on this page · raw message

Signature · 1 line

A bad analogy is like a leaky screwdriver -- Richard Braakman