Skip to contentSkip to conversation

clean up for ftp conntrack helper

PATCH In review Pablo Neira 2 participants 3 messages Apr 14, 2004 – Apr 15, 2004

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 1/1 clean up for ftp conntrack helper

Hi Harald,

I split the the array of struct ftp_search in two, one for patterns to match in IP_CT_DIR_ORIGINAL, and the other for IP_CT_DIR_REPLY. This way we save four if's conditions and we only loop twice for every direction (worst case).

I also move up if (found==0) which means that pattern was not found. I consider that this is the general case. Am I missing anything?

Some considerations, my patch of the new expectation API must be applied before applying this patch. btw, this patch could have some problems with:

http://lists.netfilter.org/pipermail/netfilter-devel/2004-March/014516.html

that patch was already reviewed by Patrick and you. Anyway that one is simple so I have no problem in merge them to this one.

regards,
Pablo

net/ipv4/netfilter/ip_conntrack_ftp.c +31 -30
@@ -51,38 +51,38 @@
 static int try_epsv_response(const char *, size_t, u_int32_t [], char);
 
 static struct ftp_search {
-	enum ip_conntrack_dir dir;
 	const char *pattern;
 	size_t plen;
 	char skip;
 	char term;
 	enum ip_ct_ftp_type ftptype;
 	int (*getnum)(const char *, size_t, u_int32_t[], char);
-} search[] = {
-	{
-		IP_CT_DIR_ORIGINAL,
+} search[IP_CT_DIR_MAX][2] = {
+	{ 
+		{
 		"PORT",	sizeof("PORT") - 1, ' ', '\r',
 		IP_CT_FTP_PORT,
 		try_rfc959,
-	},
-	{
-		IP_CT_DIR_REPLY,
-		"227 ",	sizeof("227 ") - 1, '(', ')',
-		IP_CT_FTP_PASV,
-		try_rfc959,
-	},
-	{
-		IP_CT_DIR_ORIGINAL,
+		},
+		{
 		"EPRT", sizeof("EPRT") - 1, ' ', '\r',
 		IP_CT_FTP_EPRT,
 		try_eprt,
-	},
-	{
-		IP_CT_DIR_REPLY,
+		}
+	}
+	,
+	{ 
+		{
+		"227 ", sizeof("227 ") - 1, '(', ')',
+		IP_CT_FTP_PASV,
+		try_rfc959,
+		},
+		{
 		"229 ", sizeof("229 ") - 1, '(', ')',
 		IP_CT_FTP_EPSV,
 		try_epsv_response,
-	},
+		}
+	}
 };
 
 static int try_number(const char *data, size_t dlen, u_int32_t array[],

@@ -314,33 +314,34 @@
 	array[2] = (ntohl(ct->tuplehash[dir].tuple.src.ip) >> 8) & 0xFF;
 	array[3] = ntohl(ct->tuplehash[dir].tuple.src.ip) & 0xFF;
 
-	for (i = 0; i < ARRAY_SIZE(search); i++) {
-		if (search[i].dir != dir) continue;
+	for (i = 0; i < ARRAY_SIZE(search[dir]); i++) {
 
 		found = find_pattern(ftp_buffer, skb->len - dataoff,
-				     search[i].pattern,
-				     search[i].plen,
-				     search[i].skip,
-				     search[i].term,
+				     search[dir][i].pattern,
+				     search[dir][i].plen,
+				     search[dir][i].skip,
+				     search[dir][i].term,
 				     &matchoff, &matchlen,
 				     array,
-				     search[i].getnum);
+				     search[dir][i].getnum);
 		if (found) break;
 	}
-	if (found == -1) {
+
+	if (found == 0) { /* General case: No match */
+		ret = NF_ACCEPT;
+		goto out;
+		
+	} else if (found == -1) {
 		/* We don't usually drop packets.  After all, this is
 		   connection tracking, not packet filtering.
 		   However, it is necessary for accurate tracking in
 		   this case. */
 		if (net_ratelimit())
 			printk("conntrack_ftp: partial %s %u+%u\n",
-			       search[i].pattern,
+			       search[dir][i].pattern,
 			       ntohl(tcph.seq), datalen);
 		ret = NF_DROP;
 		goto out;
-	} else if (found == 0) { /* No match */
-		ret = NF_ACCEPT;
-		goto out;
 	}
 
 	DEBUGP("conntrack_ftp: match `%.*s' (%u bytes at %u)\n",

@@ -361,7 +362,7 @@
 	    == ct->tuplehash[dir].tuple.src.ip) {
 		exp->seq = ntohl(tcph.seq) + matchoff;
 		exp_ftp_info->len = matchlen;
-		exp_ftp_info->ftptype = search[i].ftptype;
+		exp_ftp_info->ftptype = search[dir][i].ftptype;
 		exp_ftp_info->port = array[4] << 8 | array[5];
 	} else {
 		/* Enrico Scholz's passive FTP to partially RNAT'd ftp
#1 lore raw
Quoted 1 line · Pablo Neira

that patch was already reviewed by Patrick and you. Anyway that one is simple so I have no problem in merge them to this one.

The patch looks good to me. I've merged it with the datalen patch and fixed intendation/removed some newlines, I will add this after giving it a run tomorrow.

Regards,
Patrick

Quoted 3 lines

regards,
Pablo

===== net/ipv4/netfilter/ip_conntrack_ftp.c 1.18 vs edited =====

net/ipv4/netfilter/ip_conntrack_ftp.c +40 -41
@@ -51,38 +51,37 @@
 static int try_epsv_response(const char *, size_t, u_int32_t [], char);
 
 static struct ftp_search {
-	enum ip_conntrack_dir dir;
 	const char *pattern;
 	size_t plen;
 	char skip;
 	char term;
 	enum ip_ct_ftp_type ftptype;
 	int (*getnum)(const char *, size_t, u_int32_t[], char);
-} search[] = {
-	{
-		IP_CT_DIR_ORIGINAL,
-		"PORT",	sizeof("PORT") - 1, ' ', '\r',
-		IP_CT_FTP_PORT,
-		try_rfc959,
-	},
-	{
-		IP_CT_DIR_REPLY,
-		"227 ",	sizeof("227 ") - 1, '(', ')',
-		IP_CT_FTP_PASV,
-		try_rfc959,
-	},
-	{
-		IP_CT_DIR_ORIGINAL,
-		"EPRT", sizeof("EPRT") - 1, ' ', '\r',
-		IP_CT_FTP_EPRT,
-		try_eprt,
-	},
-	{
-		IP_CT_DIR_REPLY,
-		"229 ", sizeof("229 ") - 1, '(', ')',
-		IP_CT_FTP_EPSV,
-		try_epsv_response,
+} search[IP_CT_DIR_MAX][2] = {
+	{ 
+		{
+			"PORT",	sizeof("PORT") - 1, ' ', '\r',
+			IP_CT_FTP_PORT,
+			try_rfc959,
+		},
+		{
+			"EPRT", sizeof("EPRT") - 1, ' ', '\r',
+			IP_CT_FTP_EPRT,
+			try_eprt,
+		}
 	},
+	{ 
+		{
+			"227 ", sizeof("227 ") - 1, '(', ')',
+			IP_CT_FTP_PASV,
+			try_rfc959,
+		},
+		{
+			"229 ", sizeof("229 ") - 1, '(', ')',
+			IP_CT_FTP_EPSV,
+			try_epsv_response,
+		}
+	}
 };
 
 static int try_number(const char *data, size_t dlen, u_int32_t array[],

@@ -281,7 +280,7 @@
 	datalen = skb->len - dataoff;
 
 	LOCK_BH(&ip_ftp_lock);
-	skb_copy_bits(skb, dataoff, ftp_buffer, skb->len - dataoff);
+	skb_copy_bits(skb, dataoff, ftp_buffer, datalen);
 
 	old_seq_aft_nl_set = ct_ftp_info->seq_aft_nl_set[dir];
 	old_seq_aft_nl = ct_ftp_info->seq_aft_nl[dir];

@@ -314,33 +313,33 @@
 	array[2] = (ntohl(ct->tuplehash[dir].tuple.src.ip) >> 8) & 0xFF;
 	array[3] = ntohl(ct->tuplehash[dir].tuple.src.ip) & 0xFF;
 
-	for (i = 0; i < ARRAY_SIZE(search); i++) {
-		if (search[i].dir != dir) continue;
-
-		found = find_pattern(ftp_buffer, skb->len - dataoff,
-				     search[i].pattern,
-				     search[i].plen,
-				     search[i].skip,
-				     search[i].term,
+	for (i = 0; i < ARRAY_SIZE(search[dir]); i++) {
+		found = find_pattern(ftp_buffer, datalen,
+				     search[dir][i].pattern,
+				     search[dir][i].plen,
+				     search[dir][i].skip,
+				     search[dir][i].term,
 				     &matchoff, &matchlen,
 				     array,
-				     search[i].getnum);
+				     search[dir][i].getnum);
 		if (found) break;
 	}
-	if (found == -1) {
+
+	if (found == 0) {
+		/* General case: No match */
+		ret = NF_ACCEPT;
+		goto out;
+	} else if (found == -1) {
 		/* We don't usually drop packets.  After all, this is
 		   connection tracking, not packet filtering.
 		   However, it is necessary for accurate tracking in
 		   this case. */
 		if (net_ratelimit())
 			printk("conntrack_ftp: partial %s %u+%u\n",
-			       search[i].pattern,
+			       search[dir][i].pattern,
 			       ntohl(tcph.seq), datalen);
 		ret = NF_DROP;
 		goto out;
-	} else if (found == 0) { /* No match */
-		ret = NF_ACCEPT;
-		goto out;
 	}
 
 	DEBUGP("conntrack_ftp: match `%.*s' (%u bytes at %u)\n",

@@ -354,7 +353,7 @@
 	    == ct->tuplehash[dir].tuple.src.ip) {
 		exp->seq = ntohl(tcph.seq) + matchoff;
 		exp_ftp_info->len = matchlen;
-		exp_ftp_info->ftptype = search[i].ftptype;
+		exp_ftp_info->ftptype = search[dir][i].ftptype;
 		exp_ftp_info->port = array[4] << 8 | array[5];
 	} else {
 		/* Enrico Scholz's passive FTP to partially RNAT'd ftp
#2 lore raw
In reply to Pablo Neira
Quoted 2 lines · Pablo Neira

Some considerations, my patch of the new expectation API must be applied before applying this patch. btw, this patch could have some problems with:

I've added your patch to pom-ng, with another minor change (C99 initializers for ftp_search array). BTW, it also applies/works well without the expect API patch. Thanks Pablo.

Regards,
Patrick