Skip to contentSkip to conversation

MASQUERADE not flushing conntracks on ip change

PATCH In review Phil Oester 4 participants 13 messages Nov 2, 2004 – Nov 8, 2004

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Phil Oester

author
#0 lore raw

PATCH 1/1 MASQUERADE not flushing conntracks on ip change

About 14 months ago, MASQUERADE target was modified to only flush conntracks on interface up/down if the IP address changed [1]. Unfortunately, there were a few problems with this change:

1) ina->ifa_address was used as the IP address of the interface for comparison purposes. This works great for ethernet interfaces since ifa_address and ifa_local are identical. But on ppp interfaces ifa_address is the address of the remote side, not the local side

2) dev->ifindex was used to determine whether the interface which cycled matches the interface the conntrack is masquerading out. Again, this works fine for ethernet (static ifindex), but not at all for ppp which uses sequentially increasing interface indexes. The ifindex associated with pppX increments on each up/down cycle

3) even if #2 were not true, in a scenario where multiple ppp interfaces are utilized, the order in which they are cycled makes the ifindex comparison haphazard

The below patch addresses these issues by returning to the old behavior for ppp interfaces: flush all conntracks masquerading out that interface on device down. I can think of no other foolproof way to ensure the proper conntracks get destroyed.

This fixes Bugzilla #227

Phil

[1] http://linux.bkbits.net:8080/linux-2.5/diffs/net/ipv4/netfilter/[email protected]?nav=index.html|src/.|src/net|src/net/ipv4|src/net/ipv4/netfilter|hist/net/ipv4/netfilter/ipt_MASQUERADE.c

net/ipv4/netfilter/ipt_MASQUERADE.c +57 -5
diff -ru linux-orig/net/ipv4/netfilter/ipt_MASQUERADE.c linux-new/net/ipv4/netfilter/ipt_MASQUERADE.c
@@ -118,16 +118,28 @@
 }
 
 static inline int
+index_cmp(const struct ip_conntrack *i, void *ifindex)
+{
+	int ret = 0;
+
+	READ_LOCK(&masq_lock);
+	ret = (i->nat.masq_index == (int)(long)ifindex);
+	READ_UNLOCK(&masq_lock);
+
+	return ret;
+}
+
+static inline int
 device_cmp(const struct ip_conntrack *i, void *_ina)
 {
 	int ret = 0;
 	struct in_ifaddr *ina = _ina;
 
 	READ_LOCK(&masq_lock);
-	/* If it's masquerading out this interface with a different address,
-	   or we don't know the new address of this interface. */
+	/* If it's masquerading out this interface with a
+	 * different or unknown address, drop conntrack. */
 	if (i->nat.masq_index == ina->ifa_dev->dev->ifindex
-	    && i->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip != ina->ifa_address)
+	    && i->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip != ina->ifa_local)
 		ret = 1;
 	READ_UNLOCK(&masq_lock);
 

@@ -146,14 +158,46 @@
 	return 0;
 }
 
+static int
+masq_device_event(struct notifier_block *this,
+		  unsigned long event,
+		  void *ptr)
+{
+	struct net_device *dev = ptr;
+
+	/* Point-to-Point interfaces don't use static interface
+	 * indexes, so conntracks associated with these devices
+	 * must be cleared on device down. */
+	if (event == NETDEV_DOWN && (dev->flags & IFF_POINTOPOINT)) {
+		IP_NF_ASSERT(dev->ifindex != 0);
+
+		ip_ct_selective_cleanup(index_cmp, (void *)(long)dev->ifindex);
+	}
+	return NOTIFY_DONE;
+}
+
 static int masq_inet_event(struct notifier_block *this,
 			   unsigned long event,
 			   void *ptr)
 {
+	struct net_device *dev = ((struct in_ifaddr *)ptr)->ifa_dev->dev;
+
+	/* Point-to-Point interfaces don't use static interface
+	 * indexes, so conntracks associated with these devices
+	 * must be cleared on IP address deletion. */
+	if (event == NETDEV_DOWN && (dev->flags & IFF_POINTOPOINT)) {
+		/* Search entire table for conntracks which were
+		 * associated with the device and forget them. */
+		IP_NF_ASSERT(dev->ifindex != 0);
+
+		ip_ct_selective_cleanup(index_cmp, (void *)(long)dev->ifindex);
+		return NOTIFY_DONE;
+	}
+
 	/* For some configurations, interfaces often come back with
 	 * the same address.  If not, clean up old conntrack
 	 * entries. */
-	if (event == NETDEV_UP)
+	if (event == NETDEV_UP && !(dev->flags & IFF_POINTOPOINT))
 		ip_ct_selective_cleanup(device_cmp, ptr);
 	else if (event == NETDEV_DOWN)
 		ip_ct_selective_cleanup(connect_unassure, ptr);

@@ -161,6 +205,10 @@
 	return NOTIFY_DONE;
 }
 
+static struct notifier_block masq_dev_notifier = {
+	.notifier_call  = masq_device_event,
+};
+
 static struct notifier_block masq_inet_notifier = {
 	.notifier_call	= masq_inet_event,
 };

@@ -178,9 +226,12 @@
 
 	ret = ipt_register_target(&masquerade);
 
-	if (ret == 0)
+	if (ret == 0) {
+		/* Register for device down reports */
+		register_netdevice_notifier(&masq_dev_notifier);
 		/* Register IP address change reports */
 		register_inetaddr_notifier(&masq_inet_notifier);
+	}
 
 	return ret;
 }

@@ -188,6 +239,7 @@
 static void __exit fini(void)
 {
 	ipt_unregister_target(&masquerade);
+	unregister_netdevice_notifier(&masq_dev_notifier);
 	unregister_inetaddr_notifier(&masq_inet_notifier);	
 }
#1 lore raw
Quoted 15 lines · Phil Oester

About 14 months ago, MASQUERADE target was modified to only flush conntracks on interface up/down if the IP address changed [1]. Unfortunately, there were a few problems with this change:

1) ina->ifa_address was used as the IP address of the interface for comparison purposes. This works great for ethernet interfaces since ifa_address and ifa_local are identical. But on ppp interfaces ifa_address is the address of the remote side, not the local side

2) dev->ifindex was used to determine whether the interface which cycled matches the interface the conntrack is masquerading out. Again, this works fine for ethernet (static ifindex), but not at all for ppp which uses sequentially increasing interface indexes. The ifindex associated with pppX increments on each up/down cycle

Quoted 10 lines

3) even if #2 were not true, in a scenario where multiple ppp interfaces are utilized, the order in which they are cycled makes the ifindex comparison haphazard

The below patch addresses these issues by returning to the old behavior for ppp interfaces: flush all conntracks masquerading out that interface on device down. I can think of no other foolproof way to ensure the proper conntracks get destroyed.

I think we should revert to the old behaviour for all interfaces. When MASQUERADE was using a route-lookup for selecting the source there were good reasons for using MASQUERADE on devices with statically configured adresses, and some people (like me) still do it today. Simple adding a second IP address to an interface flushes all MASQUERADEDED conntracks on the device, which is not very nice. The optimization was meant for ppp devices anyway, if we can't use it there I don't see much reason to keep it.

Opinions anyone ?

Regards
Patrick

author
#2 lore raw
Quoted 10 lines · Patrick McHardy

I think we should revert to the old behaviour for all interfaces. When MASQUERADE was using a route-lookup for selecting the source there were good reasons for using MASQUERADE on devices with statically configured adresses, and some people (like me) still do it today. Simple adding a second IP address to an interface flushes all MASQUERADEDED conntracks on the device, which is not very nice. The optimization was meant for ppp devices anyway, if we can't use it there I don't see much reason to keep it.

Opinions anyone ?

It is nice that a powercycle of your router/switch/dslmodem/cablemodem/etc doesn't cause lost conntracks. The optimization is of value here.

Given these events are infrequent, and not in any fast path, any reason why the behaviour shouldn't be maintained for ethernet since it works there?

Phil

#3 lore raw
Quoted 16 lines · Phil Oester
Quoted 9 lines · Patrick McHardy

I think we should revert to the old behaviour for all interfaces. When MASQUERADE was using a route-lookup for selecting the source there were good reasons for using MASQUERADE on devices with statically configured adresses, and some people (like me) still do it today. Simple adding a second IP address to an interface flushes all MASQUERADEDED conntracks on the device, which is not very nice. The optimization was meant for ppp devices anyway, if we can't use it there I don't see much reason to keep it.

Quoted 3 lines

Opinions anyone ?

Quoted 8 lines

It is nice that a powercycle of your router/switch/dslmodem/cablemodem/etc doesn't cause lost conntracks. The optimization is of value here.

Given these events are infrequent, and not in any fast path, any reason why the behaviour shouldn't be maintained for ethernet since it works there?

Ok, I agree it is still useful, but using the inetaddr_notifier gives false positives when more than one IP address is added to the interface.

Regards
Patrick

#4 lore raw
Quoted 2 lines · Patrick McHardy

Ok, I agree it is still useful, but using the inetaddr_notifier gives false positives when more than one IP address is added to the interface.

Which is not an environment MASQUERADE is designed for, so it should be acceptable.

There is always the fallback to plain SNAT with ctnetlink for cleaning up stale connections when needed.

Regards
Henrik

#5 lore raw
Quoted 7 lines · Henrik Nordstrom
Quoted 2 lines · Patrick McHardy

Ok, I agree it is still useful, but using the inetaddr_notifier gives false positives when more than one IP address is added to the interface.

Which is not an environment MASQUERADE is designed for, so it should be acceptable.

But we do try to handle such an environment gracefully by using rt_gateway for the inet_select_addr call.

Quoted 1 line

There is always the fallback to plain SNAT with ctnetlink for cleaning up stale connections when needed.

The problem is the opposite, living conntracks are killed when more than one IP address is added to the interface.

Phil mentioned Router/switch/dslmodem/cablemodem power cycles. Contrary to what I said earlier, I don't see what value this optimization might have. Router/switch powercycle doesn't matter The optimization doesn't work for dslmodems (ppp devices), with cablemodems you don't loose your IP, except in the very unlucky situation that your DHCP lease times out while you are disconnected and you get a different one afterwards.

Regards
Patrick

author
#6 lore raw
Quoted 10 lines · Patrick McHardy

The problem is the opposite, living conntracks are killed when more than one IP address is added to the interface.

Phil mentioned Router/switch/dslmodem/cablemodem power cycles. Contrary to what I said earlier, I don't see what value this optimization might have. Router/switch powercycle doesn't matter The optimization doesn't work for dslmodems (ppp devices), with cablemodems you don't loose your IP, except in the very unlucky situation that your DHCP lease times out while you are disconnected and you get a different one afterwards.

My dsl modem attaches to my pc via ethernet cable. If I powercycle the modem, ethx up/down, conntracks get lost. Same goes for cablemodems.

Unconditionally flushing conntracks on dev down still doesn't seem the right behaviour. But I agree Patrick that the ip address add case is problematic.

I'm investigating another possible solution at the moment...

Phil

#7 lore raw
In reply to Patrick McHardy
Quoted 4 lines · Patrick McHardy
Quoted 1 line

There is always the fallback to plain SNAT with ctnetlink for cleaning up stale connections when needed.

The problem is the opposite, living conntracks are killed when more than one IP address is added to the interface.

Which is when the fallback to SNAT with ctnetlink for more controller cleaning is suitable.. and also the fact that you have more than one IP also suggest that you quite likely are not in the scope of configurations MASQUREADE is designed for (dynamic IP).

I am not saying that the current code is correct, only that I see no reason why MASQUERADE should consider being overly friendly to people having multiple IP addresses on their dynamic IP interface.

Regards
Henrik

#8 lore raw
Quoted 3 lines · Henrik Nordstrom

I am not saying that the current code is correct, only that I see no reason why MASQUERADE should consider being overly friendly to people having multiple IP addresses on their dynamic IP interface.

I totally agree with Henrik in this issue. But we relly need to document it. Maybe printk() some warning in case somebody adds a second address to an interface that uses MASQUERADE (from within the notifier)?

Quoted 2 lines

Regards
Henrik

Signature · 5 lines
- Harald Welte <[email protected]>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie
#9 lore raw
Quoted 4 lines · Harald Welte

On Fri, Nov 05, 2004 at 12:40:03AM +0100, Henrik Nordstrom wrote:

Quoted 9 lines
Quoted 3 lines

I am not saying that the current code is correct, only that I see no reason why MASQUERADE should consider being overly friendly to people having multiple IP addresses on their dynamic IP interface.

I totally agree with Henrik in this issue. But we relly need to document it. Maybe printk() some warning in case somebody adds a second address to an interface that uses MASQUERADE (from within the notifier)?

We only know is someone adds a true secondary address, not multiple primaries, otherwise we could just ignore it. Anyway, I agree we don't need to be overly friendly, I just don't see a case where this optimization does something useful. On ethernet devices, why delete the IP (if it didn't change) or set the interface down in the first place ? On ppp-interfaces, it doesn't work. Phil mentioned powercycling his dsl-/cablemodem would set his eth-interface down. I find that hard to believe, so I assume he didn't literally meant "my", but picked a bad example.

So, can anyone think of a setup where this optimization does work ?

Regards
Patrick

author
#10 lore raw
Quoted 11 lines · Patrick McHardy

We only know is someone adds a true secondary address, not multiple primaries, otherwise we could just ignore it. Anyway, I agree we don't need to be overly friendly, I just don't see a case where this optimization does something useful. On ethernet devices, why delete the IP (if it didn't change) or set the interface down in the first place ? On ppp-interfaces, it doesn't work. Phil mentioned powercycling his dsl-/cablemodem would set his eth-interface down. I find that hard to believe, so I assume he didn't literally meant "my", but picked a bad example.

So, can anyone think of a setup where this optimization does work ?

Yes, I picked a bad example...I'll put down the crackpipe before typing next time.

Anyway...below is what I'm thinking about now, which will handle both the ppp case and the 'ip addr add' case. Unfortunately, it's not working on the ppp case due to some (IMO) unexpected behaviour from inet_confirm_addr.

Comments?

Phil

net/ipv4/netfilter/ipt_MASQUERADE.c +8 -10
@@ -118,16 +118,15 @@
 }
 
 static inline int
-device_cmp(const struct ip_conntrack *i, void *_ina)
+device_cmp(const struct ip_conntrack *i, void *junk)
 {
 	int ret = 0;
-	struct in_ifaddr *ina = _ina;
 
 	READ_LOCK(&masq_lock);
-	/* If it's masquerading out this interface with a different address,
-	   or we don't know the new address of this interface. */
-	if (i->nat.masq_index == ina->ifa_dev->dev->ifindex
-	    && i->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip != ina->ifa_address)
+	/* If masquerading this conntrack but the masquerading ip
+	   no longer exists locally, drop conntrack. */
+	if (i->nat.masq_index && !(inet_confirm_addr(NULL, 0,
+	    i->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip, RT_SCOPE_HOST)))
 		ret = 1;
 	READ_UNLOCK(&masq_lock);
 

@@ -150,11 +149,10 @@
 			   unsigned long event,
 			   void *ptr)
 {
-	/* For some configurations, interfaces often come back with
-	 * the same address.  If not, clean up old conntrack
-	 * entries. */
+	/* In some configurations, interfaces come back with the
+	 * same address.  If not, clean up old conntrack entries. */
 	if (event == NETDEV_UP)
-		ip_ct_selective_cleanup(device_cmp, ptr);
+		ip_ct_selective_cleanup(device_cmp, NULL);
 	else if (event == NETDEV_DOWN)
 		ip_ct_selective_cleanup(connect_unassure, ptr);
#11 lore raw
In reply to Patrick McHardy
Quoted 4 lines · Patrick McHardy

I think we should revert to the old behaviour for all interfaces. When MASQUERADE was using a route-lookup for selecting the source there were good reasons for using MASQUERADE on devices with statically configured adresses, and some people (like me) still do it today.

If this is what you want create another "Automatic SNAT" target without the flush logics.

Presonally my opinion is that SNAT does the job just fine in static configurations, but I can imagine a few complex multilink setups where automatic selection of the SNAT IP may be preferable.

Regards
Henrik

#12 lore raw
Quoted 2 lines · Henrik Nordstrom

If this is what you want create another "Automatic SNAT" target without the flush logics.

The old code is broken, that's why I don't want to keep it. I'm perfectly fine with a working solution.

Regards
Patrick