Skip to contentSkip to conversation

return value of conntrack protocol helper functions and a macro for stats

PATCH In review Pablo Neira 2 participants 3 messages Aug 5, 2004 – Aug 21, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 return value of conntrack protocol helper functions and a macro for stats

Hi,

This patch applies to last Harald's patch bombing. It's based on Martin's idea of removing negative values in conntrack protocol helper functions. Please see:

http://lists.netfilter.org/pipermail/netfilter-devel/2004-June/015769.html

Now functions packet and error return CONNTRACK_CONT to let the packet continue its travel through the conntrack system. It also add a macro to increase vars used for stats.

regards,
Pablo

include/linux/netfilter_ipv4/ip_conntrack.h +6 -1
diff -u -r1.2 ip_conntrack.h
@@ -10,6 +10,10 @@
 #include <linux/compiler.h>
 #include <asm/atomic.h>
 
+/* Protocol specific functions (packet and error) return CONNTRACK_CONT
+ * to perform no action on a packet. */
+#define CONNTRACK_CONT -1
+
 enum ip_conntrack_info
 {
 	/* Part of an established connection (either direction). */
@@ -303,12 +307,13 @@
 	unsigned int insert_failed;
 	unsigned int drop;
 	unsigned int early_drop;
-	unsigned int icmp_error;
+	unsigned int error;
 	unsigned int expect_new;
 	unsigned int expect_create;
 	unsigned int expect_delete;
 };
 
+#define CONNTRACK_STAT_INC(count) (__get_cpu_var(ip_conntrack_stat).count++)
 
 /* eg. PROVIDES_CONNTRACK(ftp); */
 #define PROVIDES_CONNTRACK(name)                        \
net/ipv4/netfilter/ip_conntrack_core.c +24 -23

132 lines not on this page · raw message

net/ipv4/netfilter/ip_conntrack_proto_generic.c +1 -1
diff -u -r1.2 ip_conntrack_proto_generic.c
@@ -53,7 +53,7 @@
 		  enum ip_conntrack_info ctinfo)
 {
 	ip_ct_refresh_acct(conntrack, ctinfo, skb, ip_ct_generic_timeout);
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
 
 /* Called when a new connection for this protocol found. */
net/ipv4/netfilter/ip_conntrack_proto_icmp.c +7 -7
diff -u -r1.2 ip_conntrack_proto_icmp.c
@@ -102,7 +102,7 @@
 		ip_ct_refresh_acct(ct, ctinfo, skb, ip_ct_icmp_timeout);
 	}
 
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
 
 /* Called when a new connection for this protocol found. */
@@ -193,7 +193,7 @@
 
 	/* Update skb to refer to this connection */
 	skb->nfct = &h->ctrack->infos[*ctinfo];
-	return -NF_ACCEPT;
+	return NF_ACCEPT;
 }
 
 /* Small and modified version of icmp_rcv */
@@ -208,7 +208,7 @@
 		if (LOG_INVALID(IPPROTO_ICMP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
 				      "ip_ct_icmp: short packet ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 
 	/* See ip_conntrack_proto_tcp.c */
@@ -222,13 +222,13 @@
 		if (LOG_INVALID(IPPROTO_ICMP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				      "ip_ct_icmp: bad HW ICMP checksum ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	case CHECKSUM_NONE:
 		if ((u16)csum_fold(skb_checksum(skb, 0, skb->len, 0))) {
 			if (LOG_INVALID(IPPROTO_ICMP))
 				nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 					      "ip_ct_icmp: bad ICMP checksum ");
-			return -NF_ACCEPT;
+			return NF_ACCEPT;
 		}
 	default:
 		break;
@@ -245,7 +245,7 @@
 		if (LOG_INVALID(IPPROTO_ICMP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
 				      "ip_ct_icmp: invalid ICMP type ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 
 	/* Need to track icmp error message? */
@@ -254,7 +254,7 @@
 	    && icmph.type != ICMP_TIME_EXCEEDED
 	    && icmph.type != ICMP_PARAMETERPROB
 	    && icmph.type != ICMP_REDIRECT)
-		return NF_ACCEPT;
+		return CONNTRACK_CONT;
 
 	return icmp_error_message(skb, ctinfo, hooknum);
 }
net/ipv4/netfilter/ip_conntrack_proto_tcp.c +13 -13
diff -u -r1.2 ip_conntrack_proto_tcp.c
@@ -769,7 +769,7 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				"ip_ct_tcp: short packet ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
   	}
   
 	/* Not whole TCP header or malformed packet */
@@ -777,7 +777,7 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				"ip_ct_tcp: truncated/malformed packet ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
   
 	/* Checksum invalid? Ignore.
@@ -793,7 +793,7 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_tcp: bad TCP checksum ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 
 	/* Check TCP flags. */
@@ -802,10 +802,10 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_tcp: invalid TCP flag combination ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
 
 static inline void copy_whole_tcp_header(const struct sk_buff *skb,
@@ -864,7 +864,7 @@
 		    	if (del_timer(&conntrack->timeout))
 		    		conntrack->timeout.function((unsigned long)
 		    					    conntrack);
-		    	return -NF_DROP;
+		    	return NF_DROP;
 		}
 		conntrack->proto.tcp.last_index = index;
 		conntrack->proto.tcp.last_dir = dir;
@@ -874,7 +874,7 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_tcp: invalid SYN (ignored) ");
-		return NF_ACCEPT;
+		return CONNTRACK_CONT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
 		DEBUGP("ip_ct_tcp: Invalid dir=%i index=%u ostate=%u\n",
@@ -884,7 +884,7 @@
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_tcp: invalid state ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	case TCP_CONNTRACK_SYN_SENT:
 		if (old_state >= TCP_CONNTRACK_TIME_WAIT) {	
 		    	/* Attempt to reopen a closed connection.
@@ -893,7 +893,7 @@
 		    	if (del_timer(&conntrack->timeout))
 		    		conntrack->timeout.function((unsigned long)
 		    					    conntrack);
-		    	return -NF_REPEAT;
+		    	return NF_REPEAT;
 		}
 		break;
 	case TCP_CONNTRACK_CLOSE:
@@ -908,7 +908,7 @@
 			if (LOG_INVALID(IPPROTO_TCP))
 				nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 					  "ip_ct_tcp: invalid RST (ignored) ");
-			return NF_ACCEPT;
+			return CONNTRACK_CONT;
 		}
 		/* Just fall trough */
 	default:
@@ -919,7 +919,7 @@
 	if (!tcp_in_window(&conntrack->proto.tcp, dir, &index, 
 			   skb, iph, tcph)) {
 		WRITE_UNLOCK(&tcp_lock);
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 	/* From now on we have got in-window packets */
 	
@@ -950,7 +950,7 @@
 			if (del_timer(&conntrack->timeout))
 				conntrack->timeout.function((unsigned long)
 							    conntrack);
-			return NF_ACCEPT;
+			return CONNTRACK_CONT;
 		}
 	} else if (!test_bit(IPS_ASSURED_BIT, &conntrack->status)
 		   && (old_state == TCP_CONNTRACK_SYN_RECV
@@ -963,7 +963,7 @@
 	}
 	ip_ct_refresh_acct(conntrack, ctinfo, skb, timeout);
 
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
  
   /* Called when a new connection for this protocol found. */
net/ipv4/netfilter/ip_conntrack_proto_udp.c +6 -6
diff -u -r1.2 ip_conntrack_proto_udp.c
@@ -74,7 +74,7 @@
 	} else
 		ip_ct_refresh_acct(conntrack, ctinfo, skb, ip_ct_udp_timeout);
 
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
 
 /* Called when a new connection for this protocol found. */
@@ -95,7 +95,7 @@
 		if (LOG_INVALID(IPPROTO_UDP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_udp: short packet ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 	
 	/* Truncated/malformed packets */
@@ -103,12 +103,12 @@
 		if (LOG_INVALID(IPPROTO_UDP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_udp: truncated/malformed packet ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 	
 	/* Packet with no checksum */
 	if (!hdr.check)
-		return NF_ACCEPT;
+		return CONNTRACK_CONT;
 
 	/* Checksum invalid? Ignore.
 	 * We skip checking packets on the outgoing path
@@ -122,10 +122,10 @@
 		if (LOG_INVALID(IPPROTO_UDP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
 				  "ip_ct_udp: bad UDP checksum ");
-		return -NF_ACCEPT;
+		return NF_ACCEPT;
 	}
 	
-	return NF_ACCEPT;
+	return CONNTRACK_CONT;
 }
 
 struct ip_conntrack_protocol ip_conntrack_protocol_udp =
#1 lore raw
Quoted 1 line · Pablo Neira

Hi,

Hi Pablo

I've finally had the opportunity to go through my netfilter-devel backlog.

Quoted 9 lines

This patch applies to last Harald's patch bombing. It's based on Martin's idea of removing negative values in conntrack protocol helper functions. Please see:

http://lists.netfilter.org/pipermail/netfilter-devel/2004-June/015769.html

Now functions packet and error return CONNTRACK_CONT to let the packet continue its travel through the conntrack system. It also add a macro to increase vars used for stats.

I like this patch, just a few comments below.

include/linux/netfilter_ipv4/ip_conntrack.h line 314

Quoted 10 lines
include/linux/netfilter_ipv4/ip_conntrack.h +1 -1
@@ -303,12 +307,13 @@
 	unsigned int insert_failed;
 	unsigned int drop;
 	unsigned int early_drop;
-	unsigned int icmp_error;
+	unsigned int error;
 	unsigned int expect_new;
 	unsigned int expect_create;
 	unsigned int expect_delete;
 };

First I wondered why you changed this.

include/linux/netfilter_ipv4/ip_conntrack.h line 316

Quoted 1 line
include/linux/netfilter_ipv4/ip_conntrack.h +1 -0
+#define CONNTRACK_STAT_INC(count) (__get_cpu_var(ip_conntrack_stat).count++)

I like the macro, it's a lot cleaner.

net/ipv4/netfilter/ip_conntrack_core.c line 788

Quoted 4 lines
net/ipv4/netfilter/ip_conntrack_core.c +0 -0
@@ -786,41 +786,42 @@
 	/* It may be an special packet, error, unclean...
 	 * inverse of the return code tells to the netfilter
 	 * core what to do with the packet. */

Remove part of the comment above?

net/ipv4/netfilter/ip_conntrack_core.c line 796

Quoted 12 lines
net/ipv4/netfilter/ip_conntrack_core.c +7 -4
-	if (proto->error != NULL 
-	    && (ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0) {
-		__get_cpu_var(ip_conntrack_stat).icmp_error++;
-		return -ret;
+	if (proto->error != NULL) {
+		ret = proto->error(*pskb, &ctinfo, hooknum);
+		if (ret != CONNTRACK_CONT) {
+			CONNTRACK_STAT_INC(error);
+			CONNTRACK_STAT_INC(invalid);
+			return ret;
+		}
 	}

Now I see why the name was changed in the struct, it's not correct anymore (was when I wrote it :)

This patch should be submitted before 2.6.9 is out.

I'll update the ctstat program to reflect this change. There's also some more interesting events in the now submitted tcp-windowtracking patch that we could use for statistics.

I've modified the ctstat program to be able to handle additional fields in /proc/net/ip_conntrack_stat if it would be increased in the future in order to report more stats.

I'll see if I can get ctstat distributed in the same channel as rtstat, I thought it was distributed with iproute2 but maybe that's added in the RH iproute2 package as I didn't see rtstat in the Debian iproute package.

Signature · 1 line
/Martin
#2 lore raw
Quoted 10 lines · Martin Josefsson

Review line 314

Quoted 10 lines
(quoted diff) +1 -1
@@ -303,12 +307,13 @@
 	unsigned int insert_failed;
 	unsigned int drop;
 	unsigned int early_drop;
-	unsigned int icmp_error;
+	unsigned int error;
 	unsigned int expect_new;
 	unsigned int expect_create;
 	unsigned int expect_delete;
 };

There's a piece missing:
(hand-made, ymmv)

net/ipv4/netfilter/ip_conntrack_standalone.c +1 -1
@@ -334,6 +334,6 @@
 		   st->insert_failed,
 		   st->drop,
 		   st->early_drop,
-		   st->icmp_error,
+		   st->error,
 
 		   st->expect_new,

st->expect_create,

Signature · 1 line
/Martin