Skip to contentSkip to conversation

Fix expectation creation

PATCH In review Pablo Neira 2 participants 5 messages Aug 1, 2005 – Aug 2, 2005 1 message not in the archive

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

PATCH 6/7 Fix expectation creation

Expectation creation is broken, now it works again.

net/ipv4/netfilter/ip_conntrack_netlink.c +26 -20
Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
@@ -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
#1 lore raw
Quoted 1 line · Pablo Neira

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.

Signature · 5 lines
- Harald Welte <[email protected]>                 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
#3 lore raw
In reply to a message not in the archive
Quoted 7 lines · Harald Welte
Quoted 1 line · Pablo Neira

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.

Signature · 5 lines
- Harald Welte <[email protected]>                 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
#4 lore raw
Quoted 11 lines · Harald Welte
Quoted 8 lines · Harald Welte
Quoted 1 line · Pablo Neira

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.

Quoted 4 lines

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.

Quoted 3 lines

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

#5 lore raw
Quoted 3 lines · Pablo Neira

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.

Quoted 4 lines

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.

Signature · 5 lines
- Harald Welte <[email protected]>                 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