[PATCH 6/7] Fix expectation creation

[PATCH 6/7] Fix expectation creation

[Pablo Neira]

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

Expectation creation is broken, now it works again.

[-- Attachment #2: 06fix-expect-new.patch --]
[-- Type: text/x-patch, Size: 2832 bytes --]

Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- netfilter-2.6.14.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-08-01 18:21:18.000000000 +0200
+++ netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-08-01 18:21:22.000000000 +0200
@@ -1415,9 +1415,9 @@
 }
 
 static int
-ctnetlink_create_expect(struct nfattr *cda[])
+ctnetlink_create_expect(struct nfattr *cda[], struct ip_conntrack_tuple *master)
 {
-	struct ip_conntrack_tuple tuple, mask, master_tuple;
+	struct ip_conntrack_tuple tuple, mask;
 	struct ip_conntrack_tuple_hash *h = NULL;
 	struct ip_conntrack_expect *exp;
 	struct ip_conntrack *ct;
@@ -1428,23 +1428,12 @@
 	err = ctnetlink_parse_tuple(cda, &tuple, CTA_EXPECT_TUPLE);
 	if (err < 0)
 		return err;
-	err = ctnetlink_parse_tuple(cda, &tuple, CTA_EXPECT_MASK);
-	if (err < 0)
-		return err;
-
-	if (cda[CTA_TUPLE_ORIG-1])
-		err = ctnetlink_parse_tuple(cda, &master_tuple, CTA_TUPLE_ORIG);
-	else if (cda[CTA_TUPLE_REPLY-1])
-		err = ctnetlink_parse_tuple(cda, &master_tuple, 
-					    CTA_TUPLE_REPLY);
-	else
-		return -EINVAL;
-
+	err = ctnetlink_parse_tuple(cda, &mask, CTA_EXPECT_MASK);
 	if (err < 0)
 		return err;
 
 	/* Look for master conntrack of this expectation */
-	h = ip_conntrack_find_get(&master_tuple, NULL);
+	h = ip_conntrack_find_get(master, NULL);
 	if (!h)
 		return -ENOENT;
 	ct = tuplehash_to_ctrack(h);
@@ -1478,16 +1467,30 @@
 ctnetlink_new_expect(struct sock *ctnl, struct sk_buff *skb,
 		     struct nlmsghdr *nlh, struct nfattr *cda[], int *errp)
 {
-	struct ip_conntrack_tuple tuple;
+	struct nfattr *tb[CTA_EXPECT_MAX];
+	struct ip_conntrack_tuple tuple, master;
 	struct ip_conntrack_expect *exp;
 	int err = 0;
 
-	DEBUGP("entered %s\n", __FUNCTION__);	
+	DEBUGP("entered %s\n", __FUNCTION__);
 
-	if (!cda[CTA_EXPECT_TUPLE-1] || !cda[CTA_EXPECT_MASK-1])
+	if (!cda[CTA_EXPECT-1])
+		return -EINVAL;
+
+	if (cda[CTA_TUPLE_ORIG-1])
+		err = ctnetlink_parse_tuple(cda, &master, CTA_TUPLE_ORIG);
+	else if (cda[CTA_TUPLE_REPLY-1])
+		err = ctnetlink_parse_tuple(cda, &master, CTA_TUPLE_REPLY);
+	else
 		return -EINVAL;
 
-	err = ctnetlink_parse_tuple(cda, &tuple, CTA_EXPECT_TUPLE);
+	if (err < 0)
+		return err;
+
+	if (nfattr_parse_nested(tb, CTA_EXPECT_MAX, cda[CTA_EXPECT-1]) < 0)
+		goto nfattr_failure;
+
+	err = ctnetlink_parse_tuple(tb, &tuple, CTA_EXPECT_TUPLE);
 	if (err < 0)
 		return err;
 
@@ -1498,7 +1501,7 @@
 		write_unlock_bh(&ip_conntrack_lock);
 		err = -ENOENT;
 		if (nlh->nlmsg_flags & NLM_F_CREATE)
-			err = ctnetlink_create_expect(cda);
+			err = ctnetlink_create_expect(tb, &master);
 		return err;
 	}
 
@@ -1510,6 +1513,9 @@
 	DEBUGP("leaving\n");
 	
 	return err;
+
+nfattr_failure:
+	return -1;
 }
 
 #ifdef CONFIG_IP_NF_CONNTRACK_EVENTS

Re: [PATCH 6/7] Fix expectation creation

[Harald Welte]

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

On Mon, Aug 01, 2005 at 07:06:53PM +0200, Pablo Neira wrote:
> Expectation creation is broken, now it works again.

I'll look into it, but please try to provide more verbose changelog
messages for your patches - this makes review on my side easier (and I
would have to add them before submitting to Dave anyway). Thanks.
-- 
- Harald Welte <laforge@netfilter.org>                 http://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: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 6/7] Fix expectation creation

[Harald Welte]

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

On Tue, Aug 02, 2005 at 09:45:56AM +0200, Harald Welte wrote:
> On Mon, Aug 01, 2005 at 07:06:53PM +0200, Pablo Neira wrote:
> > Expectation creation is broken, now it works again.
> 
> well, you also make it a nested attribute now.  I'm not sure whether
> this is the way to go.  And if you do, there are certainly some CTA
> values from nfnetlink_conntrack.h that need to be removed with the same
> patch.

Mh. I'm still somewhat undecided on this issue.  We don't encapsulate
'struct ip_conntrack' in one nested attribute, but with your patch we do
it for 'struct ip_conntrack_expect'.  This sounds a bit inconsistent to
me.

Independent of this discussion, could you please submit two patches:
1) one that just fixes the bug(s) that you currently see with expect_create
2) one patch incremental to '1)' that adds nesting of expectations.

I'm trying to find out pro's and con's of nesting in this particular
case.  Is there any functionality that we gain by nesting it?

I'd rather see a seperate list of CTA_... attributes for CONNTRACK and
EXPET message types.  

-- 
- Harald Welte <laforge@netfilter.org>                 http://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: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 6/7] Fix expectation creation

[Pablo Neira]

Harald Welte wrote:
> On Tue, Aug 02, 2005 at 09:45:56AM +0200, Harald Welte wrote:
> 
>>On Mon, Aug 01, 2005 at 07:06:53PM +0200, Pablo Neira wrote:
>>
>>>Expectation creation is broken, now it works again.
>>
>>well, you also make it a nested attribute now.  I'm not sure whether
>>this is the way to go.  And if you do, there are certainly some CTA
>>values from nfnetlink_conntrack.h that need to be removed with the same
>>patch.
> 
> Mh. I'm still somewhat undecided on this issue.  We don't encapsulate
> 'struct ip_conntrack' in one nested attribute, but with your patch we do
> it for 'struct ip_conntrack_expect'.  This sounds a bit inconsistent to
> me.

We could move CTA_EXPECT_[TUPLE|MASK] to ctattr_type, kill 
CTA_EXPECT_[ID|TIMEOUT] and use CTA_[ID|TIMEOUT] but then we'll be in 
trouble. See that during expectation creation we send the information 
related with master conntrack and the expectation. We need a way to 
separate what information is related with the conntrack, and what is 
related with the expectation. Nesting a conntrack inside something like 
CTA_CONNTRACK is too much I think.

See that an expectation doesn't have any meaning by itself without a 
conntrack. That's why I decided to keep it as a nested attributes inside 
a conntrack. Because of the implicit relationship that links them.

> Independent of this discussion, could you please submit two patches:
> 1) one that just fixes the bug(s) that you currently see with expect_create
 > 2) one patch incremental to '1)' that adds nesting of expectations.

Sorry, I don't understand how I can do this yet. I see that 1) and 2) 
can't be split since the bug is that (based on the current structure of 
conntrack netlink attributes), we need to parse CTA_EXPECT nested 
attributes and we aren't doing it.

--
Pablo

Re: [PATCH 6/7] Fix expectation creation

[Harald Welte]

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

On Tue, Aug 02, 2005 at 01:31:28PM +0200, Pablo Neira wrote:
> We could move CTA_EXPECT_[TUPLE|MASK] to ctattr_type, kill
> CTA_EXPECT_[ID|TIMEOUT] and use CTA_[ID|TIMEOUT] but then we'll be in
> trouble. 

Please look at the current netfilter-2.6.14 tree and tell me if you see
any problems with that.

> See that an expectation doesn't have any meaning by itself without a
> conntrack.  That's why I decided to keep it as a nested attributes
> inside a conntrack.  Because of the implicit relationship that links
> them.

it's merely a pointer to the conntrack that can be identified by a
single tuple.

-- 
- Harald Welte <laforge@netfilter.org>                 http://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: Type: application/pgp-signature, Size: 189 bytes --]