MASQUERADE handling of device events

To achieve this, two changes were required to core networking code (thus the linux-net cc):

1) export inet_confirm_addr

2) change inet_ifa_match to use ifa_local instead of ifa_address. Since ifa_local != ifa_address on ppp interfaces, inet_ifa_match could not be used to verify ppp interface addresses without this change.

Comments?

The below patch addresses these issues by changing from using ifindex comparisons to verifying that the masquerading ip still exists on the box.

To achieve this, two changes were required to core networking code (thus the linux-net cc):

1) export inet_confirm_addr

2) change inet_ifa_match to use ifa_local instead of ifa_address. Since ifa_local != ifa_address on ppp interfaces, inet_ifa_match could not be used to verify ppp interface addresses without this change.

As discussed in recent days, MASQUERADE target handling of device events is broken in a couple of ways:

1) when ppp interfaces cycle, conntracks with old ip addresses are not flushed

2) an 'ip addr add' on an interface used for masquerading flushes all conntracks associated with that interface

The below patch addresses these issues by changing from using ifindex comparisons to verifying that the masquerading ip still exists on the box.

------------------------------------------------------------------------

diff -ru linux-orig/include/linux/inetdevice.h linux-diff/include/linux/inetdevice.h

include/linux/inetdevice.h +1 -1

@@ -114,7 +114,7 @@
 
 static __inline__ int inet_ifa_match(u32 addr, struct in_ifaddr *ifa)
 {
-	return !((addr^ifa->ifa_address)&ifa->ifa_mask);
+	return !((addr^ifa->ifa_local)&ifa->ifa_mask);
 }
 
 /*

diff -ru linux-orig/net/ipv4/devinet.c linux-diff/net/ipv4/devinet.c

net/ipv4/devinet.c +1 -0

@@ -1493,6 +1493,7 @@
 
 EXPORT_SYMBOL(devinet_ioctl);
 EXPORT_SYMBOL(in_dev_finish_destroy);
+EXPORT_SYMBOL(inet_confirm_addr);
 EXPORT_SYMBOL(inet_select_addr);
 EXPORT_SYMBOL(inetdev_by_index);
 EXPORT_SYMBOL(register_inetaddr_notifier);

diff -ru linux-orig/net/ipv4/netfilter/ipt_MASQUERADE.c linux-diff/net/ipv4/netfilter/ipt_MASQUERADE.c

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

@@ -118,16 +118,15 @@
 }
 

static inline int

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

-device_cmp(const struct ip_conntrack *i, void *_ina)
+device_cmp(const struct ip_conntrack *i, void *junk)

{
	int ret = 0;

net/ipv4/netfilter/ipt_MASQUERADE.c +0 -1

-	struct in_ifaddr *ina = _ina;

	READ_LOCK(&masq_lock);

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

-	/* 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);

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

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

But without the ifindex comparison, a situation like this can happen:

ppp0 goes down
ppp1 goes down
ppp0 comes up again, same IP as before
ppp1 connections get killed
ppp1 comes up again

So we should keep this.

Quoted 8 lines · Patrick McHardy

But without the ifindex comparison, a situation like this can happen:

ppp0 goes down
ppp1 goes down
ppp0 comes up again, same IP as before
ppp1 connections get killed
ppp1 comes up again

Quoted 2 lines

So we should keep this.

But ifindex is meaningless on ppp interfaces - it is incremented on each cycle. So there is no way to say that the original ppp1 is the same interface as the new ppp1.

ifindex just cannot be used reliably.

Quoted 2 lines

ifindex just cannot be used reliably.

Of course, I wasn't thinking :) Then what about Henrik's suggestion, replacing masq_index by a new status bit ?

It's a great idea, and will reduce the size of struct ip_conntrack. But I think it should be done in a separate cleanup patch - really would like to get this one merged up to fix the masq issues.

Quoted 5 lines · Phil Oester

It's a great idea, and will reduce the size of struct ip_conntrack. But I think it should be done in a separate cleanup patch - really would like to get this one merged up to fix the masq issues.

Actually masq_index is still used in connect_unassure, and thus can't be removed completely. In cases where the interface goes down permanently, clearing the assured bit makes sense, so guess this behaviour should be maintained.

You're right. I have to admit, I'm not too happy about the unpredictable behaviour we get with this patch and multiple ppp devices. So one last attempt to convince people. The old behaviour was to kill conntracks once the device goes down. I think killing conntracks when the IP is deleted makes more sense. Since the IP has to be deleted manually, except when

the device goes away, people can simply not delete IP addresses for devices that don't go away, than nothing will get removed. pppd can be taught to keep the device alive. The attached patch adds a program alloc-ppp to pre-allocate ppp-devices and teaches pppd to attach to them. The device never goes away, if ppp doesn't delete the IP address the conntracks won't be killed. It could easily be integrated in a more handy way in pppd. So this could also be done entirely in userspace, without the unpredictable behaviour.

Quoted 5 lines · Patrick McHardy

You're right. I have to admit, I'm not too happy about the unpredictable behaviour we get with this patch and multiple ppp devices. So one last attempt to convince people. The old behaviour was to kill conntracks once the device goes down. I think killing conntracks when the IP is deleted makes more sense. Since the IP has to be deleted manually, except when

Quoted 10 lines

the device goes away, people can simply not delete IP addresses for devices that don't go away, than nothing will get removed. pppd can be taught to keep the device alive. The attached patch adds a program alloc-ppp to pre-allocate ppp-devices and teaches pppd to attach to them. The device never goes away, if ppp doesn't delete the IP address the conntracks won't be killed. It could easily be integrated in a more handy way in pppd. So this could also be done entirely in userspace, without the unpredictable behaviour.

Is this the unpredictable behaviour you refer to:

ppp0 down -> clear assured bit on ppp0 conntracks
ppp1 down -> clear assured bit on ppp1 conntracks
ppp0 up with same ip -> all ppp1 conntracks get cleared since that ip
no longer exists on box
ppp1 up with same ip -> loser

Not much we can do about that except revert to the old behaviour of dropping all conntracks on interface down, but that does seem silly for 'mostly static' pppoe users.

Obviously I'd vote for the 'friendlier' method, but bottom line is current behaviour is broken and should be fixed somehow...

Dave: would the two changes below be acceptable for you?

Quoted 1 line

1) export inet_confirm_addr

Quoted 4 lines

2) change inet_ifa_match to use ifa_local instead of ifa_address. Since ifa_local != ifa_address on ppp interfaces, inet_ifa_match could not be used to verify ppp interface addresses without this change.

As discussed in recent days, MASQUERADE target handling of device events is broken in a couple of ways:

1) when ppp interfaces cycle, conntracks with old ip addresses are not flushed

2) an 'ip addr add' on an interface used for masquerading flushes all conntracks associated with that interface

The MASQUERADE target use to destroy connections when an interface went down. We changed this to merely remove the ASSURED bit, and destroy them if the same interface came up with a different IP address. Unfortunately, as Phil Oester pointed out, that code was crap for PPP connections, since we (1) compared ifa_address instead of ifa_local, (2) identified interfaces by ifindex, which increments as a PPP device downs and ups, and (3) caused all connections to be flushed when we added an IP address.

So that code was reverted after 2.6.10-rc2.

This code stores the interface name, rather than trying to use the ifindex, and only deletes connections if *no* ifa_local on the interface matches the connection, so simply adding a new IP address is a NOOP.

Quoted 11 lines · Rusty Russell

The MASQUERADE target use to destroy connections when an interface went down. We changed this to merely remove the ASSURED bit, and destroy them if the same interface came up with a different IP address. Unfortunately, as Phil Oester pointed out, that code was crap for PPP connections, since we (1) compared ifa_address instead of ifa_local, (2) identified interfaces by ifindex, which increments as a PPP device downs and ups, and (3) caused all connections to be flushed when we added an IP address.

So that code was reverted after 2.6.10-rc2.

Quoted 4 lines

This code stores the interface name, rather than trying to use the ifindex, and only deletes connections if *no* ifa_local on the interface matches the connection, so simply adding a new IP address is a NOOP.

But even using the interface name is unreliable. Consider the user who uses multiple PPP connections. Both go down at the same time, and come back with the 'old' ppp1 now named ppp0.

Perhaps my original patch which special-cased ppp would be better?

Drawing a link between point-to-point and addresses being static is wrong, IMHO.

Quoted 2 lines · Rusty Russell

Drawing a link between point-to-point and addresses being static is wrong, IMHO.

Agreed.

This discussion is equally important on Ethernet with dynamic IP assignment. Here the interface name is defenitely static.

As already discussed very many times MASQUERADE is a best effort to handle the common "dial up like" with dynamic Internet IP assignment scenarios in an easy manner. A multiple PPP situation is more of an exception than rule to this, and in addition userspace has the option of using statically reserved PPP devices for the connections in question to make the proposed MASQUERADE fully predictable in such situations.