[PATCH 2.6] ip_nat_ftp - manip at the right place

[PATCH 2.6] ip_nat_ftp - manip at the right place

[Julian Anastasov]

	Hello,

	This is a resend/resync for v2.6.9-rc1-bk17: change the
way the ip_nat_ftp helper manipulates the packets:

- no manips => no fixup

- check the direction, do manip once and at the same time when the
headers are changed

	This is needed mostly for IPVS setups and I hope we do not
create troubles for other setups or FTP software.

Signed-off-by: Julian Anastasov <ja@ssi.bg>

diff -ur v2.6.9-rc1-bk17/linux/net/ipv4/netfilter/ip_nat_ftp.c linux/net/ipv4/netfilter/ip_nat_ftp.c
--- v2.6.9-rc1-bk17/linux/net/ipv4/netfilter/ip_nat_ftp.c	2004-09-11 09:35:33.000000000 +0300
+++ linux/net/ipv4/netfilter/ip_nat_ftp.c	2004-09-11 10:29:38.343165344 +0300
@@ -237,17 +237,23 @@
 	unsigned int datalen;
 	int dir;
 	struct ip_ct_ftp_expect *exp_ftp_info;
+	int i, do_manip = 0;
 
 	if (!exp)
 		DEBUGP("ip_nat_ftp: no exp!!");
 
 	exp_ftp_info = &exp->help.exp_ftp_info;
 
-	/* Only mangle things once: original direction in POST_ROUTING
-	   and reply direction on PRE_ROUTING. */
+	/* Only mangle things once: for the first manip in this direction. */
 	dir = CTINFO2DIR(ctinfo);
-	if (!((hooknum == NF_IP_POST_ROUTING && dir == IP_CT_DIR_ORIGINAL)
-	      || (hooknum == NF_IP_PRE_ROUTING && dir == IP_CT_DIR_REPLY))) {
+	for (i = 0; i < info->num_manips; i++) {
+		if (info->manips[i].direction == dir) {
+			if (info->manips[i].hooknum == hooknum)
+				do_manip = 1;
+			break;
+		}
+	}
+	if (!do_manip) {
 		DEBUGP("nat_ftp: Not touching dir %s at hook %s\n",
 		       dir == IP_CT_DIR_ORIGINAL ? "ORIG" : "REPLY",
 		       hooknum == NF_IP_POST_ROUTING ? "POSTROUTING"

Re: [PATCH 2.6] ip_nat_ftp - manip at the right place

[Harald Welte]

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

On Sat, Sep 11, 2004 at 10:53:53AM +0300, Julian Anastasov wrote:
> 	Hello,
> 
> 	This is a resend/resync for v2.6.9-rc1-bk17: change the
> way the ip_nat_ftp helper manipulates the packets:
> 
> - no manips => no fixup
> 
> - check the direction, do manip once and at the same time when the
> headers are changed

I agree with this change, but shouldn't we make it consistently over all
NAT helpers?  In case you didn't check yet, and assuming that this is
applicable to other helpers as well:  Please include patches for other
protocol helpers as well.

Thanks!

> Signed-off-by: Julian Anastasov <ja@ssi.bg>
-- 
- 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 2.6] ip_nat_ftp - manip at the right place

[Julian Anastasov]

	Hello,

On Sat, 11 Sep 2004, Harald Welte wrote:

> I agree with this change, but shouldn't we make it consistently over all
> NAT helpers?  In case you didn't check yet, and assuming that this is
> applicable to other helpers as well:  Please include patches for other
> protocol helpers as well.

	I can live with the appended patch, I hope it is correct.

Signed-off-by: Julian Anastasov <ja@ssi.bg>

diff -ur v2.6.9/linux/net/ipv4/netfilter/ip_nat_core.c linux/net/ipv4/netfilter/ip_nat_core.c
--- v2.6.9/linux/net/ipv4/netfilter/ip_nat_core.c	2004-10-19 10:00:36.000000000 +0300
+++ linux/net/ipv4/netfilter/ip_nat_core.c	2004-10-24 14:52:39.334958976 +0300
@@ -728,7 +728,7 @@
 	    struct sk_buff **pskb)
 {
 	unsigned int i;
-	struct ip_nat_helper *helper;
+	struct ip_nat_helper *helper = NULL;
 	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
 	int proto = (*pskb)->nh.iph->protocol;
 
@@ -751,9 +751,9 @@
 				READ_UNLOCK(&ip_nat_lock);
 				return NF_DROP;
 			}
+			helper = info->helper;
 		}
 	}
-	helper = info->helper;
 	READ_UNLOCK(&ip_nat_lock);
 
 	if (helper) {
diff -ur v2.6.9/linux/net/ipv4/netfilter/ip_nat_ftp.c linux/net/ipv4/netfilter/ip_nat_ftp.c
--- v2.6.9/linux/net/ipv4/netfilter/ip_nat_ftp.c	2004-10-19 10:00:36.000000000 +0300
+++ linux/net/ipv4/netfilter/ip_nat_ftp.c	2004-10-24 14:52:39.335958824 +0300
@@ -54,6 +54,8 @@
 	IP_NF_ASSERT(master);
 
 	IP_NF_ASSERT(!(info->initialized & (1<<HOOK2MANIP(hooknum))));
+	if (!(info->initialized & (1<<HOOK2MANIP(hooknum))))
+		return NF_ACCEPT;
 
 	DEBUGP("nat_expected: We have a connection!\n");
 	exp_ftp_info = &ct->master->help.exp_ftp_info;

Re: [PATCH 2.6] ip_nat_ftp - manip at the right place

[David S. Miller]

On Sat, 11 Sep 2004 10:53:53 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> 	This is a resend/resync for v2.6.9-rc1-bk17: change the
> way the ip_nat_ftp helper manipulates the packets:
> 
> - no manips => no fixup
> 
> - check the direction, do manip once and at the same time when the
> headers are changed
> 
> 	This is needed mostly for IPVS setups and I hope we do not
> create troubles for other setups or FTP software.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>

Harald and/or Rusty, please ACK/NACK this for me.

Thanks.

Re: [PATCH 2.6] ip_nat_ftp - manip at the right place

[Harald Welte]

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

On Sun, Sep 12, 2004 at 05:03:23PM -0700, David S. Miller wrote:
> On Sat, 11 Sep 2004 10:53:53 +0300 (EEST)
> Julian Anastasov <ja@ssi.bg> wrote:
> 
> > 	This is a resend/resync for v2.6.9-rc1-bk17: change the
> > way the ip_nat_ftp helper manipulates the packets:
> > 
> > - no manips => no fixup
> > 
> > - check the direction, do manip once and at the same time when the
> > headers are changed
> > 
> > 	This is needed mostly for IPVS setups and I hope we do not
> > create troubles for other setups or FTP software.
> > 
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> 
> Harald and/or Rusty, please ACK/NACK this for me.

I agree with the change (although I didn't test it here on my systems so
far).  However, as indicated in private mail to Julian, the change
should be made consistent over all helpers.

So please hold back the patch until I get back to you, thanks.

> Thanks.

-- 
- 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 2.6] ip_nat_ftp - manip at the right place

[Julian Anastasov]

	Hello,

On Tue, 14 Sep 2004, Harald Welte wrote:

> I agree with the change (although I didn't test it here on my systems so
> far).  However, as indicated in private mail to Julian, the change
> should be made consistent over all helpers.

	No private mail received yet. But I agree, it should work
for other helpers, eg. similar change can go in do_bindings instead.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH 2.6] ip_nat_ftp - manip at the right place

[David S. Miller]

On Sat, 11 Sep 2004 10:53:53 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> 	This is a resend/resync for v2.6.9-rc1-bk17: change the
> way the ip_nat_ftp helper manipulates the packets:
> 
> - no manips => no fixup
> 
> - check the direction, do manip once and at the same time when the
> headers are changed
> 
> 	This is needed mostly for IPVS setups and I hope we do not
> create troubles for other setups or FTP software.

I'm still waiting for ACK/NACK of this.  I know Rusty is travelling
and Harald is probably busy too, so I know the delay in response is not
because these guys are slackers :-)

Re: [PATCH 2.6] ip_nat_ftp - manip at the right place

[Rusty Russell]

On Tue, 2004-09-14 at 09:30, David S. Miller wrote:
> On Sat, 11 Sep 2004 10:53:53 +0300 (EEST)
> Julian Anastasov <ja@ssi.bg> wrote:
> 
> > 	This is a resend/resync for v2.6.9-rc1-bk17: change the
> > way the ip_nat_ftp helper manipulates the packets:
> > 
> > - no manips => no fixup
> > 
> > - check the direction, do manip once and at the same time when the
> > headers are changed
> > 
> > 	This is needed mostly for IPVS setups and I hope we do not
> > create troubles for other setups or FTP software.
> 
> I'm still waiting for ACK/NACK of this.  I know Rusty is travelling
> and Harald is probably busy too, so I know the delay in response is not
> because these guys are slackers :-)

Mine is because I'm porting the existing FTP testsuite to nfsim, so I
can test the patch doesn't break anything.  As far as I can tell, this
change isn't vital, so I'm not losing sleep to do it...

Thanks,
Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell