[PATCH] MASQUERADE handling of device events

[PATCH] MASQUERADE handling of device events

[Phil Oester]

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

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.

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?

Phil



[-- Attachment #2: patch-masq2 --]
[-- Type: text/plain, Size: 2466 bytes --]

diff -ru linux-orig/include/linux/inetdevice.h linux-diff/include/linux/inetdevice.h
--- linux-orig/include/linux/inetdevice.h	2004-11-04 17:32:05.573870736 -0500
+++ linux-diff/include/linux/inetdevice.h	2004-11-07 12:57:50.674323160 -0500
@@ -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
--- linux-orig/net/ipv4/devinet.c	2004-11-04 17:32:05.000000000 -0500
+++ linux-diff/net/ipv4/devinet.c	2004-11-07 12:56:27.738931256 -0500
@@ -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
--- linux-orig/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-04 17:32:05.000000000 -0500
+++ linux-diff/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-07 12:55:56.501680040 -0500
@@ -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);
 

Re: [PATCH] MASQUERADE handling of device events

[Henrik Nordstrom]

On Sun, 7 Nov 2004, Phil Oester wrote:

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

Looks fine to me, but maybe we should also utilize the fact that 
nat.masq_index with this change only needs to be a boolean flag "this 
connection is masqueraded", allowing us to cut down a little on the 
conntrack entry size.

Regards
Henrik

Re: [PATCH] MASQUERADE handling of device events

[Harald Welte]

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

[added explicit Cc to Dave]

On Sun, Nov 07, 2004 at 10:18:25AM -0800, Phil Oester wrote:
> 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):

Thanks, Phil.  

I think this solution is about the best we can get.

Any other comments?

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

> 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.
-- 
- Harald Welte <laforge@netfilter.org>             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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] MASQUERADE handling of device events

[Patrick McHardy]

Phil Oester wrote:

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

Regards
Patrick

>  
>
>------------------------------------------------------------------------
>
>diff -ru linux-orig/include/linux/inetdevice.h linux-diff/include/linux/inetdevice.h
>--- linux-orig/include/linux/inetdevice.h	2004-11-04 17:32:05.573870736 -0500
>+++ linux-diff/include/linux/inetdevice.h	2004-11-07 12:57:50.674323160 -0500
>@@ -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
>--- linux-orig/net/ipv4/devinet.c	2004-11-04 17:32:05.000000000 -0500
>+++ linux-diff/net/ipv4/devinet.c	2004-11-07 12:56:27.738931256 -0500
>@@ -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
>--- linux-orig/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-04 17:32:05.000000000 -0500
>+++ linux-diff/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-07 12:55:56.501680040 -0500
>@@ -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);
> 
>  
>

Re: [PATCH] MASQUERADE handling of device events

[Phil Oester]

On Mon, Nov 08, 2004 at 05:05:38PM +0100, Patrick McHardy wrote:
> 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.

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.

Phil

Re: [PATCH] MASQUERADE handling of device events

[Patrick McHardy]

Phil Oester wrote:

>On Mon, Nov 08, 2004 at 05:05:38PM +0100, Patrick McHardy wrote:
>
>>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.
>>
>
>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.
>
Of course, I wasn't thinking :) Then what about Henrik's suggestion,
replacing masq_index by a new status bit ?

Regards
Patrick

Re: [PATCH] MASQUERADE handling of device events

[Phil Oester]

On Mon, Nov 08, 2004 at 05:24:52PM +0100, Patrick McHardy wrote:
> >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.

Phil

Re: [PATCH] MASQUERADE handling of device events

[Phil Oester]

On Mon, Nov 08, 2004 at 08:34:57AM -0800, Phil Oester wrote:
> 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.

Phil

Re: [PATCH] MASQUERADE handling of device events

[Patrick McHardy]

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

Phil Oester wrote:

>On Mon, Nov 08, 2004 at 08:34:57AM -0800, Phil Oester wrote:
>  
>
>>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.

Regards
Patrick


[-- Attachment #2: ppp_attach.diff --]
[-- Type: text/x-patch, Size: 3438 bytes --]

diff -urN a/pppd/alloc-ppp.c b/pppd/alloc-ppp.c
--- a/pppd/alloc-ppp.c	1970-01-01 01:00:00.000000000 +0100
+++ b/pppd/alloc-ppp.c	2004-11-04 17:09:35.804341216 +0100
@@ -0,0 +1,64 @@
+
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <sys/ioctl.h>
+
+#include <asm/types.h> 
+#include <net/if.h>
+#include <net/if_arp.h>
+#include <net/route.h>
+#include <netinet/if_ether.h>
+
+
+#include <linux/ppp_defs.h>
+#include <linux/if_ppp.h>
+
+
+
+int main( int argc, char ** argv ) {
+
+    int ndev = 0;
+    int n = 0;
+    int ppp_dev_fd = 0;
+
+    if( argc != 2 ) {
+        fprintf(stderr,"use \"%s <n>\" to allocate n ppp devices\n", argv[0]);
+	exit(1);
+    }
+
+    ndev = atoi(argv[1]);
+
+    if( ndev <= 0 ) {
+        fprintf(stderr,"argument must b a number > 0 not %s\n", argv[1]);
+	exit(1);
+    }
+
+    for( n = 0; n < ndev; n ++ ) {
+        int ifunit = n;
+        int res = 0;
+	
+        if( (ppp_dev_fd = open("/dev/ppp", O_RDWR )) < 0 ) {
+	    perror("can not open /dev/ppp :");
+	    exit(2);
+        }
+
+        res = ioctl(ppp_dev_fd, PPPIOCNEWUNIT, &ifunit);
+	if( res < 0 ) {
+	    fprintf(stderr, "can not allocate ppp device %d : %s \n",
+	    	n , strerror(errno) );
+	}
+    }
+
+    while( 1 == 1 ) {
+        sleep(3600);
+    }
+
+    exit(0);
+}
+
diff -urN a/pppd/Makefile.linux b/pppd/Makefile.linux
--- a/pppd/Makefile.linux	2003-11-27 22:55:19.000000000 +0100
+++ b/pppd/Makefile.linux	2004-11-04 17:09:35.804341216 +0100
@@ -8,7 +8,7 @@
 MANDIR = $(DESTDIR)/usr/man
 INCDIR = $(DESTDIR)/usr/include
 
-TARGETS = pppd
+TARGETS = pppd alloc-ppp
 
 PPPDSRCS = main.c magic.c fsm.c lcp.c ipcp.c upap.c chap-new.c md5.c ccp.c \
 	   ecp.c ipxcp.c auth.c options.c sys-linux.c md4.c chap_ms.c \
@@ -196,10 +196,11 @@
 
 all: $(TARGETS)
 
-install: pppd
+install: pppd alloc-ppp
 	mkdir -p $(BINDIR) $(MANDIR)
 	$(EXTRAINSTALL)
 	$(INSTALL) -s -c -m 555 pppd $(BINDIR)/pppd
+	$(INSTALL) -s -c -m 555 alloc-ppp $(BINDIR)/alloc-ppp
 	if chgrp pppusers $(BINDIR)/pppd 2>/dev/null; then \
 	  chmod o-rx,u+s $(BINDIR)/pppd; fi
 	$(INSTALL) -c -m 444 pppd.8 $(MANDIR)/man8
@@ -207,6 +208,9 @@
 pppd: $(PPPDOBJS)
 	$(CC) $(CFLAGS) $(LDFLAGS) -o pppd $(PPPDOBJS) $(LIBS)
 
+alloc-ppp: alloc-ppp.o
+	$(CC) $(CFLAGS) -o alloc-ppp alloc-ppp.o
+
 srp-entry:	srp-entry.c
 	$(CC) $(CFLAGS) $(LDFLAGS) -o $@ srp-entry.c $(LIBS)
 
@@ -216,6 +220,7 @@
 
 clean:
 	rm -f $(PPPDOBJS) $(EXTRACLEAN) $(TARGETS) *~ #* core
+	rm -f alloc-ppp.o alloc-ppp
 
 depend:
 	$(CPP) -M $(CFLAGS) $(PPPDSRCS) >.depend
diff -urN a/pppd/sys-linux.c b/pppd/sys-linux.c
--- a/pppd/sys-linux.c	2004-01-13 05:05:20.000000000 +0100
+++ b/pppd/sys-linux.c	2004-11-04 17:10:06.951606112 +0100
@@ -639,6 +639,21 @@
 		warn("Couldn't set /dev/ppp to nonblock: %m");
 
 	ifunit = req_unit;
+
+	/* 
+	 * try to attach to an alread existing ppp device. We should
+	 * get an EFAULT if the ppp interface is in use by another pppd.
+	 */
+	if (ifunit >= 0) {
+		x = ioctl(ppp_dev_fd, PPPIOCATTACH , &ifunit);
+		if (x < 0) {
+			/* warn and continue to create a new device */
+			warn("Couldn't attatch to unit %d as it does not exist", req_unit);
+		} else {
+			return x;
+		}
+	}
+
 	x = ioctl(ppp_dev_fd, PPPIOCNEWUNIT, &ifunit);
 	if (x < 0 && req_unit >= 0 && errno == EEXIST) {
 		warn("Couldn't allocate PPP unit %d as it is already in use", req_unit);

Re: [PATCH] MASQUERADE handling of device events

[Phil Oester]

On Tue, Nov 09, 2004 at 12:04:47PM +0100, Patrick McHardy wrote:
> 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.

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

Phil

Re: [PATCH] MASQUERADE handling of device events

[Patrick McHardy]

Phil Oester wrote:

>On Tue, Nov 09, 2004 at 12:04:47PM +0100, Patrick McHardy wrote:
>  
>
>>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.
>>    
>>
>
>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
>  
>
Yes. It gives unpredictable behaviour and no way to avoid it.
If ppp1 comes up first - ppp0 is the loser. If only one goes
down at a time - nobody looses.

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

I think deterministic behaviour is more friendly than something
working only under certain conditions and doing strange stuff
the remaining time. I would like to hear Harald's opinion.

Regards
Patrick

Re: [PATCH] MASQUERADE handling of device events

[David S. Miller]

On Mon, 8 Nov 2004 14:50:25 +0100
Harald Welte <laforge@netfilter.org> wrote:

> Dave: would the two changes below be acceptable for you?
> 
> > 1) export inet_confirm_addr

This one I think is OK.

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

This one on the other hand is a serious semantic change
and needs more careful thought.  ifa_address is used for
a reason, and people expecting that are going to break
if we change things this way.

A study of at least the net/ipv4/devinet.c shows that this
ifa_address vs. ifa_local distinction is definitely on
purpose.  I really don't think we can make this suggested
change therefore.

Re: [PATCH] MASQUERADE handling of device events

[Rusty Russell]

On Sun, 2004-11-07 at 10:18 -0800, Phil Oester wrote:
> 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

Sorry, just caught up with this reversion in 2.6.10-rc2-bk* (was trying
to figure out why my nfsim testcase was failing: this explains it).

We had numerous complaints about the old behaviour: several people with
small (or clever?) ISPs actually got the same IP address most of the
time, and the old code was damaging them.  It also registered two
callbacks which AFAICT is unnecessary.

This patch bloats conntrack, but is worth it IMHO.  Most importantly,
Phil, does it work for you?

Rusty.
Name: Reliable Masquerading Connection Removal
Status: Compiles, Untested
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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.

Index: linux-2.6.10-rc2-bk1-Netfilter/include/linux/netfilter_ipv4/ip_conntrack.h
===================================================================
--- linux-2.6.10-rc2-bk1-Netfilter.orig/include/linux/netfilter_ipv4/ip_conntrack.h	2004-11-20 17:53:54.000000000 +1100
+++ linux-2.6.10-rc2-bk1-Netfilter/include/linux/netfilter_ipv4/ip_conntrack.h	2004-11-20 17:55:40.000000000 +1100
@@ -193,7 +193,7 @@
 		union ip_conntrack_nat_help help;
 #if defined(CONFIG_IP_NF_TARGET_MASQUERADE) || \
 	defined(CONFIG_IP_NF_TARGET_MASQUERADE_MODULE)
-		int masq_index;
+		char masq_iface[IFNAMSIZ];
 #endif
 	} nat;
 #endif /* CONFIG_IP_NF_NAT_NEEDED */
Index: linux-2.6.10-rc2-bk1-Netfilter/net/ipv4/netfilter/ipt_MASQUERADE.c
===================================================================
--- linux-2.6.10-rc2-bk1-Netfilter.orig/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-19 16:51:36.000000000 +1100
+++ linux-2.6.10-rc2-bk1-Netfilter/net/ipv4/netfilter/ipt_MASQUERADE.c	2004-11-21 13:50:02.943750896 +1100
@@ -104,7 +104,7 @@
 	}
 
 	WRITE_LOCK(&masq_lock);
-	ct->nat.masq_index = out->ifindex;
+	strcpy(ct->nat.masq_iface, out->name);
 	WRITE_UNLOCK(&masq_lock);
 
 	/* Transfer from original range. */
@@ -118,57 +118,61 @@
 }
 
 static inline int
-device_cmp(const struct ip_conntrack *i, void *ifindex)
+no_address_matches(u32 dstip, struct in_device *in_dev)
 {
-	int ret;
+	struct in_ifaddr *i;
+
+	for (i = in_dev->ifa_list; i; i = i->ifa_next)
+		if (i->ifa_local == dstip)
+			return 0;
+	return 1;
+}
+
+static inline int
+device_cmp(const struct ip_conntrack *i, void *_ina)
+{
+	int ret = 0;
+	struct in_ifaddr *ina = _ina;
 
 	READ_LOCK(&masq_lock);
-	ret = (i->nat.masq_index == (int)(long)ifindex);
+	/* If it's masquerading out this interface with an address,
+	 * which is not any of the existing ones, time to go. */
+	if (strcmp(i->nat.masq_iface, ina->ifa_dev->dev->name) == 0
+	    && no_address_matches(i->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip,
+				  ina->ifa_dev))
+		ret = 1;
 	READ_UNLOCK(&masq_lock);
 
 	return ret;
 }
 
-static int masq_device_event(struct notifier_block *this,
-			     unsigned long event,
-			     void *ptr)
-{
-	struct net_device *dev = ptr;
-
-	if (event == NETDEV_DOWN) {
-		/* Device was downed.  Search entire table for
-		   conntracks which were associated with that device,
-		   and forget them. */
-		IP_NF_ASSERT(dev->ifindex != 0);
-
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
-	}
+static inline int
+connect_unassure(const struct ip_conntrack *i, void *_ina)
+{
+	struct in_ifaddr *ina = _ina;
 
-	return NOTIFY_DONE;
+	/* We reset the ASSURED bit on all connections, so they will
+	 * get reaped under memory pressure. */
+	if (strcmp(i->nat.masq_iface, ina->ifa_dev->dev->name) == 0)
+		clear_bit(IPS_ASSURED_BIT, (unsigned long *)&i->status);
+	return 0;
 }
 
 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;
-
-	if (event == NETDEV_DOWN) {
-		/* IP address was deleted.  Search entire table for
-		   conntracks which were associated with that device,
-		   and forget them. */
-		IP_NF_ASSERT(dev->ifindex != 0);
-
-		ip_ct_selective_cleanup(device_cmp, (void *)(long)dev->ifindex);
-	}
+	/* For some configurations, interfaces often come back with
+	 * the same address.  If not, clean up old conntrack
+	 * entries. */
+	if (event == NETDEV_UP)
+		ip_ct_selective_cleanup(device_cmp, ptr);
+	else if (event == NETDEV_DOWN)
+		ip_ct_selective_cleanup(connect_unassure, ptr);
 
 	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,
 };
@@ -187,8 +191,6 @@
 	ret = ipt_register_target(&masquerade);
 
 	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);
 	}
@@ -199,7 +201,6 @@
 static void __exit fini(void)
 {
 	ipt_unregister_target(&masquerade);
-	unregister_netdevice_notifier(&masq_dev_notifier);
 	unregister_inetaddr_notifier(&masq_inet_notifier);	
 }
 

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

Re: [PATCH] MASQUERADE handling of device events

[Phil Oester]

On Sun, Nov 21, 2004 at 01:58:28PM +1100, Rusty Russell wrote:
> 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.

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.  Or consider a PopTop
PPTP server which has dozens of ppp interfaces (although using masq here
is admittedly unlikely).

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

http://lists.netfilter.org/pipermail/netfilter-devel/2004-November/017294.html

Phil

Re: [PATCH] MASQUERADE handling of device events

[Rusty Russell]

On Tue, 2004-11-23 at 13:16 -0800, Phil Oester wrote:
> On Sun, Nov 21, 2004 at 01:58:28PM +1100, Rusty Russell wrote:
> > 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.
> 
> 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.

Sure, in which case the connections will be dropped, as they would be in
the naive case.  This entire idea is best-effort, like NAT itself 8)

I think it's worth trying, at least, but obviously the core team can
override me.

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

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

Re: [PATCH] MASQUERADE handling of device events

[Henrik Nordstrom]

On Wed, 24 Nov 2004, Rusty Russell wrote:

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

Regards
Henrik

Re: [PATCH] MASQUERADE handling of device events

[Herve Eychenne]

On Wed, Nov 24, 2004 at 10:24:34AM +0100, Henrik Nordstrom wrote:

> On Wed, 24 Nov 2004, Rusty Russell wrote:

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

Being the guy who proposed the original change to Rusty during the
workshop 2003, I eventually step up to support this view. :-)
IMHO, the goal of the MASQUERADE target is trying to deal as
nicely as possible with the common cases.

I think it should be kept that way, no matter it may have some side effect
for uncommon or obscur setups, as long as the potential corner cases are
clearly documented.
The more complex scenarios should always be able to use something
else than MASQUERADE.

 Herve

-- 
 _
(°=  Hervé Eychenne
//)
v_/_ WallFire project:  http://www.wallfire.org/