[NETFILTER]: Cleanup conntrack helper API

A: Pablo Neira
D: This patch changes the conntrack helper API.  Rather than having the
D: helper allocate an expect on the stack and then have the core
D: kmalloc and memcpy, it is now the job of a helper to call 
D: ip_conntrack_expect_alloc()

include/linux/netfilter_ipv4/ip_conntrack_helper.h

@@ -35,9 +35,13 @@ extern void ip_conntrack_helper_unregister(struct ip_conntrack_helper *);
 
 extern struct ip_conntrack_helper *ip_ct_find_helper(const struct ip_conntrack_tuple *tuple);
 
+
+/* Allocate space for an expectation: this is mandatory before calling 
+   ip_conntrack_expect_related. */
+extern struct ip_conntrack_expect *ip_conntrack_expect_alloc(void);
 /* Add an expected connection: can have more than one per connection */
-extern int ip_conntrack_expect_related(struct ip_conntrack *related_to,
-				       struct ip_conntrack_expect *exp);
+extern int ip_conntrack_expect_related(struct ip_conntrack_expect *exp,
+				       struct ip_conntrack *related_to);
 extern int ip_conntrack_change_expect(struct ip_conntrack_expect *expect,
 				      struct ip_conntrack_tuple *newtuple);
 extern void ip_conntrack_unexpect_related(struct ip_conntrack_expect *exp);

net/ipv4/netfilter/ip_conntrack_amanda.c

@@ -46,10 +46,11 @@ static DECLARE_LOCK(amanda_buffer_lock);
 static int help(struct sk_buff *skb,
                 struct ip_conntrack *ct, enum ip_conntrack_info ctinfo)
 {
-	struct ip_conntrack_expect exp;
+	struct ip_conntrack_expect *exp;
 	struct ip_ct_amanda_expect *exp_amanda_info;
 	char *data, *data_limit, *tmp;
 	unsigned int dataoff, i;
+	u_int16_t port, len;
 
 	/* Only look at packets from the Amanda server */
 	if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
@@ -79,33 +80,40 @@ static int help(struct sk_buff *skb,
 		goto out;
 	data += strlen("CONNECT ");
 
-	memset(&exp, 0, sizeof(exp));
-	exp.tuple.src.ip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
-	exp.tuple.dst.ip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
-	exp.tuple.dst.protonum = IPPROTO_TCP;
-	exp.mask.src.ip = 0xFFFFFFFF;
-	exp.mask.dst.ip = 0xFFFFFFFF;
-	exp.mask.dst.protonum = 0xFFFF;
-	exp.mask.dst.u.tcp.port = 0xFFFF;
-
 	/* Only search first line. */	
 	if ((tmp = strchr(data, '\n')))
 		*tmp = '\0';
 
-	exp_amanda_info = &exp.help.exp_amanda_info;
 	for (i = 0; i < ARRAY_SIZE(conns); i++) {
 		char *match = strstr(data, conns[i]);
 		if (!match)
 			continue;
 		tmp = data = match + strlen(conns[i]);
-		exp_amanda_info->offset = data - amanda_buffer;
-		exp_amanda_info->port   = simple_strtoul(data, &data, 10);
-		exp_amanda_info->len    = data - tmp;
-		if (exp_amanda_info->port == 0 || exp_amanda_info->len > 5)
+		port = simple_strtoul(data, &data, 10);
+		len = data - tmp;
+		if (port == 0 || len > 5)
 			break;
 
-		exp.tuple.dst.u.tcp.port = htons(exp_amanda_info->port);
-		ip_conntrack_expect_related(ct, &exp);
+		exp = ip_conntrack_expect_alloc();
+		if (exp == NULL)
+			goto out;
+
+		exp->tuple.src.ip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
+		exp->tuple.dst.ip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
+		exp->tuple.dst.protonum = IPPROTO_TCP;
+		exp->mask.src.ip = 0xFFFFFFFF;
+		exp->mask.dst.ip = 0xFFFFFFFF;
+		exp->mask.dst.protonum = 0xFFFF;
+		exp->mask.dst.u.tcp.port = 0xFFFF;
+
+		exp_amanda_info = &exp->help.exp_amanda_info;
+		exp_amanda_info->offset = data - amanda_buffer;
+		exp_amanda_info->port   = port;
+		exp_amanda_info->len    = len;
+
+		exp->tuple.dst.u.tcp.port = htons(port);
+
+		ip_conntrack_expect_related(exp, ct);
 	}
 
 out:

net/ipv4/netfilter/ip_conntrack_core.c

@@ -917,11 +917,55 @@ static void expectation_timed_out(unsigned long ul_expect)
 	WRITE_UNLOCK(&ip_conntrack_lock);
 }
 
+struct ip_conntrack_expect *
+ip_conntrack_expect_alloc()
+{
+	struct ip_conntrack_expect *new;
+	
+	new = (struct ip_conntrack_expect *)
+		kmalloc(sizeof(struct ip_conntrack_expect), GFP_ATOMIC);
+	if (!new) {
+		DEBUGP("expect_related: OOM allocating expect\n");
+		return NULL;
+	}
+
+	/* tuple_cmp compares whole union, we have to initialized cleanly */
+	memset(new, 0, sizeof(struct ip_conntrack_expect));
+
+	return new;
+}
+
+static void
+ip_conntrack_expect_insert(struct ip_conntrack_expect *new,
+			   struct ip_conntrack *related_to)
+{
+	DEBUGP("new expectation %p of conntrack %p\n", new, related_to);
+	new->expectant = related_to;
+	new->sibling = NULL;
+	atomic_set(&new->use, 1);
+
+	/* add to expected list for this connection */
+	list_add(&new->expected_list, &related_to->sibling_list);
+	/* add to global list of expectations */
+
+	list_prepend(&ip_conntrack_expect_list, &new->list);
+	/* add and start timer if required */
+	if (related_to->helper->timeout) {
+		init_timer(&new->timeout);
+		new->timeout.data = (unsigned long)new;
+		new->timeout.function = expectation_timed_out;
+		new->timeout.expires = jiffies +
+					related_to->helper->timeout * HZ;
+		add_timer(&new->timeout);
+	}
+	related_to->expecting++;
+}
+
 /* Add a related connection. */
-int ip_conntrack_expect_related(struct ip_conntrack *related_to,
-				struct ip_conntrack_expect *expect)
+int ip_conntrack_expect_related(struct ip_conntrack_expect *expect,
+				struct ip_conntrack *related_to)
 {
-	struct ip_conntrack_expect *old, *new;
+	struct ip_conntrack_expect *old;
 	int ret = 0;
 
 	WRITE_LOCK(&ip_conntrack_lock);
@@ -943,7 +987,7 @@ int ip_conntrack_expect_related(struct ip_conntrack *related_to,
 		if (related_to->helper->timeout) {
 			if (!del_timer(&old->timeout)) {
 				/* expectation is dying. Fall through */
-				old = NULL;
+				goto out;
 			} else {
 				old->timeout.expires = jiffies + 
 					related_to->helper->timeout * HZ;
@@ -951,10 +995,10 @@ int ip_conntrack_expect_related(struct ip_conntrack *related_to,
 			}
 		}
 
-		if (old) {
-			WRITE_UNLOCK(&ip_conntrack_lock);
-			return -EEXIST;
-		}
+		WRITE_UNLOCK(&ip_conntrack_lock);
+		kfree(expect);
+		return -EEXIST;
+
 	} else if (related_to->helper->max_expected && 
 		   related_to->expecting >= related_to->helper->max_expected) {
 		struct list_head *cur_item;
@@ -971,6 +1015,7 @@ int ip_conntrack_expect_related(struct ip_conntrack *related_to,
 				       related_to->helper->name,
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip),
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip));
+			kfree(expect);
 			return -EPERM;
 		}
 		DEBUGP("ip_conntrack: max number of expected "
@@ -1010,37 +1055,12 @@ int ip_conntrack_expect_related(struct ip_conntrack *related_to,
 			     &expect->mask)) {
 		WRITE_UNLOCK(&ip_conntrack_lock);
 		DEBUGP("expect_related: busy!\n");
+
+		kfree(expect);
 		return -EBUSY;
 	}
-	
-	new = (struct ip_conntrack_expect *) 
-	      kmalloc(sizeof(struct ip_conntrack_expect), GFP_ATOMIC);
-	if (!new) {
-		WRITE_UNLOCK(&ip_conntrack_lock);
-		DEBUGP("expect_relaed: OOM allocating expect\n");
-		return -ENOMEM;
-	}
-	
-	DEBUGP("new expectation %p of conntrack %p\n", new, related_to);
-	memcpy(new, expect, sizeof(*expect));
-	new->expectant = related_to;
-	new->sibling = NULL;
-	atomic_set(&new->use, 1);
-	
-	/* add to expected list for this connection */	
-	list_add(&new->expected_list, &related_to->sibling_list);
-	/* add to global list of expectations */
-	list_prepend(&ip_conntrack_expect_list, &new->list);
-	/* add and start timer if required */
-	if (related_to->helper->timeout) {
-		init_timer(&new->timeout);
-		new->timeout.data = (unsigned long)new;
-		new->timeout.function = expectation_timed_out;
-		new->timeout.expires = jiffies + 
-					related_to->helper->timeout * HZ;
-		add_timer(&new->timeout);
-	}
-	related_to->expecting++;
+
+out:	ip_conntrack_expect_insert(expect, related_to);
 
 	WRITE_UNLOCK(&ip_conntrack_lock);
 

net/ipv4/netfilter/ip_conntrack_ftp.c

@@ -256,8 +256,8 @@ static int help(struct sk_buff *skb,
 	int dir = CTINFO2DIR(ctinfo);
 	unsigned int matchlen, matchoff;
 	struct ip_ct_ftp_master *ct_ftp_info = &ct->help.ct_ftp_info;
-	struct ip_conntrack_expect expect, *exp = &expect;
-	struct ip_ct_ftp_expect *exp_ftp_info = &exp->help.exp_ftp_info;
+	struct ip_conntrack_expect *exp;
+	struct ip_ct_ftp_expect *exp_ftp_info;
 
 	unsigned int i;
 	int found = 0;
@@ -346,8 +346,15 @@ static int help(struct sk_buff *skb,
 	DEBUGP("conntrack_ftp: match `%.*s' (%u bytes at %u)\n",
 	       (int)matchlen, data + matchoff,
 	       matchlen, ntohl(tcph.seq) + matchoff);
-	       
-	memset(&expect, 0, sizeof(expect));
+
+	/* Allocate expectation which will be inserted */
+	exp = ip_conntrack_expect_alloc();
+	if (exp == NULL) {
+		ret = NF_ACCEPT;
+		goto out;
+	}
+
+	exp_ftp_info = &exp->help.exp_ftp_info;
 
 	/* Update the ftp info */
 	if (htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3])
@@ -389,7 +396,7 @@ static int help(struct sk_buff *skb,
 	exp->expectfn = NULL;
 
 	/* Ignore failure; should only happen with NAT */
-	ip_conntrack_expect_related(ct, &expect);
+	ip_conntrack_expect_related(exp, ct);
 	ret = NF_ACCEPT;
  out:
 	UNLOCK_BH(&ip_ftp_lock);

net/ipv4/netfilter/ip_conntrack_irc.c

@@ -106,8 +106,8 @@ static int help(struct sk_buff *skb,
 	struct tcphdr tcph;
 	char *data, *data_limit;
 	int dir = CTINFO2DIR(ctinfo);
-	struct ip_conntrack_expect expect, *exp = &expect;
-	struct ip_ct_irc_expect *exp_irc_info = &exp->help.exp_irc_info;
+	struct ip_conntrack_expect *exp;
+	struct ip_ct_irc_expect *exp_irc_info = NULL;
 
 	u_int32_t dcc_ip;
 	u_int16_t dcc_port;
@@ -190,8 +190,12 @@ static int help(struct sk_buff *skb,
 
 				continue;
 			}
-			
-			memset(&expect, 0, sizeof(expect));
+
+			exp = ip_conntrack_expect_alloc();
+			if (exp == NULL)
+				goto out;
+
+			exp_irc_info = &exp->help.exp_irc_info;
 
 			/* save position of address in dcc string,
 			 * necessary for NAT */
@@ -218,7 +222,7 @@ static int help(struct sk_buff *skb,
 				NIPQUAD(exp->tuple.dst.ip),
 				ntohs(exp->tuple.dst.u.tcp.port));
 
-			ip_conntrack_expect_related(ct, &expect);
+			ip_conntrack_expect_related(exp, ct);
 
 			goto out;
 		} /* for .. NUM_DCCPROTO */

net/ipv4/netfilter/ip_conntrack_standalone.c

@@ -591,6 +591,7 @@ EXPORT_SYMBOL(ip_ct_refresh);
 EXPORT_SYMBOL(ip_ct_find_proto);
 EXPORT_SYMBOL(__ip_ct_find_proto);
 EXPORT_SYMBOL(ip_ct_find_helper);
+EXPORT_SYMBOL(ip_conntrack_expect_alloc);
 EXPORT_SYMBOL(ip_conntrack_expect_related);
 EXPORT_SYMBOL(ip_conntrack_change_expect);
 EXPORT_SYMBOL(ip_conntrack_unexpect_related);

net/ipv4/netfilter/ip_conntrack_tftp.c

@@ -44,7 +44,7 @@ static int tftp_help(struct sk_buff *skb,
 		     enum ip_conntrack_info ctinfo)
 {
 	struct tftphdr tftph;
-	struct ip_conntrack_expect exp;
+	struct ip_conntrack_expect *exp;
 
 	if (skb_copy_bits(skb, skb->nh.iph->ihl * 4 + sizeof(struct udphdr),
 			  &tftph, sizeof(tftph)) != 0)
@@ -57,19 +57,22 @@ static int tftp_help(struct sk_buff *skb,
 		DEBUGP("");
 		DUMP_TUPLE(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
 		DUMP_TUPLE(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
-		memset(&exp, 0, sizeof(exp));
 
-		exp.tuple = ct->tuplehash[IP_CT_DIR_REPLY].tuple;
-		exp.mask.src.ip = 0xffffffff;
-		exp.mask.dst.ip = 0xffffffff;
-		exp.mask.dst.u.udp.port = 0xffff;
-		exp.mask.dst.protonum = 0xffff;
-		exp.expectfn = NULL;
+		exp = ip_conntrack_expect_alloc();
+		if (exp == NULL)
+			return NF_ACCEPT;
+
+		exp->tuple = ct->tuplehash[IP_CT_DIR_REPLY].tuple;
+		exp->mask.src.ip = 0xffffffff;
+		exp->mask.dst.ip = 0xffffffff;
+		exp->mask.dst.u.udp.port = 0xffff;
+		exp->mask.dst.protonum = 0xffff;
+		exp->expectfn = NULL;
 
 		DEBUGP("expect: ");
-		DUMP_TUPLE(&exp.tuple);
-		DUMP_TUPLE(&exp.mask);
-		ip_conntrack_expect_related(ct, &exp);
+		DUMP_TUPLE(&exp->tuple);
+		DUMP_TUPLE(&exp->mask);
+		ip_conntrack_expect_related(exp, ct);
 		break;
 	default:
 		DEBUGP("Unknown opcode\n");