IPv6: conntrack: Use protocol-related initialization routine to initial queues of IPv6 connection track

PATCH In review Shan Wei 5 participants 8 messages Jan 26, 2010 – Oct 30, 2010

Subsystems (4)

Networking Network drivers netfilter IPv4/IPv6

lore

Share & tools

lore

Shan Wei

author

2010-01-26 02:31 UTC #0 lore raw

PATCH 1/2 IPv6: conntrack: Use protocol-related initialization routine to initial queues of IPv6 connection track

IPv6 connection track and IPv6 stack separately use a different queue to manage received fragments. The former uses nf_ct_frag6_queue structure, the latter uses frag_queue structure.

When creating new queue for IPv6 connection track, ip6_frag_init() that belongs to IPv6 stack is called to initial nf_ct_frag6_queue structure. This broken the saddr&daddr member in nf_ct_frag6_queue, and then hash value generated by nf_hashfn() is not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.

The patch fixes the bug with protocol-related initialization routine. The patch-set have been tested.

Signed-off-by Shan Wei <[email protected]>

---
 include/net/ipv6.h                      |    1 -
 net/ipv6/netfilter/nf_conntrack_reasm.c |   13 ++++++++++++-
 net/ipv6/reassembly.c                   |    3 +--
 3 files changed, 13 insertions(+), 4 deletions(-)

include/net/ipv6.h +0 -1

index cbd768b..a7112da 100644
@@ -364,7 +364,6 @@ struct ip6_create_arg {
 	struct in6_addr *dst;
 };
 
-void ip6_frag_init(struct inet_frag_queue *q, void *a);
 
 static inline int ipv6_addr_any(const struct in6_addr *a)
 {

net/ipv6/netfilter/nf_conntrack_reasm.c +12 -1

index 66b6161..4a61d14 100644
@@ -146,6 +146,17 @@ static void nf_ct_frag6_evictor(void)
 	local_bh_enable();
 }
 
+static void nf_ct_queue_init(struct inet_frag_queue *q, void *a)
+{
+	struct nf_ct_frag6_queue *fq;
+	struct ip6_create_arg *arg = a;
+
+	fq = container_of(q, struct nf_ct_frag6_queue, q);
+	fq->id = arg->id;
+	ipv6_addr_copy(&fq->saddr, arg->src);
+	ipv6_addr_copy(&fq->daddr, arg->dst);
+}
+
 static int nf_ct_frag_match(struct inet_frag_queue *q, void *a)
 {
 	struct nf_ct_frag6_queue *fq;

@@ -672,7 +683,7 @@ void nf_ct_frag6_output(unsigned int hooknum, struct sk_buff *skb,
 int nf_ct_frag6_init(void)
 {
 	nf_frags.hashfn = nf_hashfn;
-	nf_frags.constructor = ip6_frag_init;
+	nf_frags.constructor = nf_ct_frag_init;
 	nf_frags.destructor = NULL;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);

net/ipv6/reassembly.c +1 -2

index 2fa4355..9f9b6a2 100644
@@ -157,7 +157,7 @@ static inline void frag_kfree_skb(struct netns_frags *nf,
 	kfree_skb(skb);
 }
 
-void ip6_frag_init(struct inet_frag_queue *q, void *a)
+static void ip6_frag_init(struct inet_frag_queue *q, void *a)
 {
 	struct frag_queue *fq = container_of(q, struct frag_queue, q);
 	struct ip6_create_arg *arg = a;

@@ -167,7 +167,6 @@ void ip6_frag_init(struct inet_frag_queue *q, void *a)
 	ipv6_addr_copy(&fq->saddr, arg->src);
 	ipv6_addr_copy(&fq->daddr, arg->dst);
 }
-EXPORT_SYMBOL(ip6_frag_init);
 
 /* Destruction primitives. */

Signature · 1 line

1.6.3.3

David Miller

maintainer

2010-01-26 07:32 UTC #1 lore raw

From: Shan Wei <[email protected]>
Date: Tue, 26 Jan 2010 10:31:10 +0800

Quoted 10 lines

IPv6 connection track and IPv6 stack separately use a different queue to manage received fragments. The former uses nf_ct_frag6_queue structure, the latter uses frag_queue structure.
When creating new queue for IPv6 connection track, ip6_frag_init() that belongs to IPv6 stack is called to initial nf_ct_frag6_queue structure. This broken the saddr&daddr member in nf_ct_frag6_queue, and then hash value generated by nf_hashfn() is not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.

Quoted 4 lines

The patch fixes the bug with protocol-related initialization routine. The patch-set have been tested.
Signed-off-by Shan Wei <[email protected]>

This breakage was recently introduced by:

commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d

Author: Patrick McHardy <[email protected]>
Date:   Tue Dec 15 16:59:18 2009 +0100

ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery

Currently the same reassembly queue might be used for packets reassembled by conntrack in different positions in the stack (PREROUTING/LOCAL_OUT), as well as local delivery. This can cause "packet jumps" when the fragment completing a reassembled packet is queued from a different position in the stack than the previous ones.

Add a "user" identifier to the reassembly queue key to seperate the queues of each caller, similar to what we do for IPv4.

Signed-off-by Patrick McHardy <[email protected]>

Shan Wei

author

2010-01-26 12:40 UTC #2 lore raw

David Miller wrote, at 01/26/2010 03:32 PM:

Quoted 18 lines

From: Shan Wei <[email protected]>
Date: Tue, 26 Jan 2010 10:31:10 +0800
Quoted 10 lines
IPv6 connection track and IPv6 stack separately use a different queue to manage received fragments. The former uses nf_ct_frag6_queue structure, the latter uses frag_queue structure.
When creating new queue for IPv6 connection track, ip6_frag_init() that belongs to IPv6 stack is called to initial nf_ct_frag6_queue structure. This broken the saddr&daddr member in nf_ct_frag6_queue, and then hash value generated by nf_hashfn() is not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.
Quoted 4 lines
The patch fixes the bug with protocol-related initialization routine. The patch-set have been tested.
Signed-off-by Shan Wei <[email protected]>

Quoted 7 lines

This breakage was recently introduced by:
commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d
Author: Patrick McHardy <[email protected]>
Date:   Tue Dec 15 16:59:18 2009 +0100
ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery

Yes, this patch adds user member to frag_queue structure，but not to nf_ct_frag6_queue structure.

Please ignore the patch-set.
Can you apply the following patch(bug-fix) to your net-tree?

--
[PATCH]IPv6: conntrack: Add member of user to nf_ct_frag6_queue structure

The commit 0b5ccb2(title:ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery) has broken the saddr&&daddr member of nf_ct_frag6_queue when creating new queue. And then hash value generated by nf_hashfn() was not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.

The patch fixes the bug with adding member of user to nf_ct_frag6_queue structure.

Signed-off-by Shan Wei <[email protected]>

---
 net/ipv6/netfilter/nf_conntrack_reasm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/ipv6/netfilter/nf_conntrack_reasm.c +1 -0

index 312c20a..624a548 100644
@@ -63,6 +63,7 @@ struct nf_ct_frag6_queue
 	struct inet_frag_queue	q;
 
 	__be32			id;		/* fragment id		*/
+	u32			user;
 	struct in6_addr		saddr;
 	struct in6_addr		daddr;

Signature · 7 lines

1.6.3.3

 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick McHardy

Acked-by

2010-01-26 12:46 UTC #3 lore raw

Quoted 20 lines · Shan Wei

David Miller wrote, at 01/26/2010 03:32 PM:
Quoted 19 lines
From: Shan Wei <[email protected]>
Date: Tue, 26 Jan 2010 10:31:10 +0800
Quoted 10 lines
IPv6 connection track and IPv6 stack separately use a different queue to manage received fragments. The former uses nf_ct_frag6_queue structure, the latter uses frag_queue structure.
When creating new queue for IPv6 connection track, ip6_frag_init() that belongs to IPv6 stack is called to initial nf_ct_frag6_queue structure. This broken the saddr&daddr member in nf_ct_frag6_queue, and then hash value generated by nf_hashfn() is not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.
Quoted 4 lines
The patch fixes the bug with protocol-related initialization routine. The patch-set have been tested.
Signed-off-by Shan Wei <[email protected]>
This breakage was recently introduced by:

Quoted 7 lines

Quoted 5 lines
commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d
Author: Patrick McHardy <[email protected]>
Date:   Tue Dec 15 16:59:18 2009 +0100
ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery
Yes, this patch adds user member to frag_queue structure，but not to nf_ct_frag6_queue structure.

Oops, sorry. Not sure why I missed this, I've successfully tested that change multiple times.

Quoted 12 lines

Please ignore the patch-set.
Can you apply the following patch(bug-fix) to your net-tree?
--
[PATCH]IPv6: conntrack: Add member of user to nf_ct_frag6_queue structure
The commit 0b5ccb2(title:ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery) has broken the saddr&&daddr member of nf_ct_frag6_queue when creating new queue. And then hash value generated by nf_hashfn() was not equal with that generated by fq_find(). So, a new received fragment can't be inserted to right queue.

Quoted 3 lines

The patch fixes the bug with adding member of user to nf_ct_frag6_queue structure.
Signed-off-by Shan Wei <[email protected]>

Acked-by Patrick McHardy <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2010-01-26 13:11 UTC #4 lore raw

From: Patrick McHardy <[email protected]>
Date: Tue, 26 Jan 2010 13:46:46 +0100

Quoted 2 lines

Oops, sorry. Not sure why I missed this, I've successfully tested that change multiple times.

This situation is error prone, and I don't blame you for not catching it, because these common ipv6 fragmentation functions are assuming things about the layout of the first few struct members of the container in which the top level data structure lives.

What should happen is that when such an assumption exists, it should be explicitly codified.

Just like how we embed struct sock_common in both struct socket and in the TCP time-wait minisockets.

Anyways, meanwhile I'll apply the fix. And yes I know it needs to go to stable too... :-)

Patrick McHardy

2010-01-26 15:46 UTC #5 lore raw

Quoted 11 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Tue, 26 Jan 2010 13:46:46 +0100
Quoted 2 lines
Oops, sorry. Not sure why I missed this, I've successfully tested that change multiple times.
This situation is error prone, and I don't blame you for not catching it, because these common ipv6 fragmentation functions are assuming things about the layout of the first few struct members of the container in which the top level data structure lives.

Quoted 5 lines

What should happen is that when such an assumption exists, it should be explicitly codified.
Just like how we embed struct sock_common in both struct socket and in the TCP time-wait minisockets.

I'll see if I can come up with something to catch this kind of mistake in the future.

Quoted 3 lines

Anyways, meanwhile I'll apply the fix. And yes I know it needs to go to stable too... :-)

Thanks :)

Pascal Hambourg

2010-10-30 18:41 UTC #6 lore raw

In reply to David Miller

Hello,

Quoted 3 lines · David Miller

Anyways, meanwhile I'll apply the fix. And yes I know it needs to go to stable too... :-)

It appears that the patch "ipv6: reassembly: use seperate reassembly queues for conntrack and local delivery" went to both the 2.6.32 and 2.6.27 stable branches, but the fix from Shan Wei went to the 2.6.32 branch only, not to the 2.6.27 branch. However I checked that it applies
on the latest 2.6.27 release, and fixes the problem.

Could someone submit it for the next 2.6.27 stable release ? Or if I have to do it, what is the process ?
Thanks.

Yasuyuki KOZAKAI

2010-01-26 12:56 UTC #7 lore raw

In reply to Shan Wei

Patches looks good to me. But nf_ct_frag_match in the this patch is added by [PATCH 2/2]. Subject mistake ?

-- Yasuyuki Kozakai

From: Shan Wei <[email protected]>
Date: Tue, 26 Jan 2010 10:31:10 +0800

Quoted 93 lines

IPv6 connection track and IPv6 stack separately use a different queue to manage received fragments. The former uses nf_ct_frag6_queue structure, the latter uses frag_queue structure.

The patch fixes the bug with protocol-related initialization routine. The patch-set have been tested.

Signed-off-by Shan Wei <[email protected]>

---
 include/net/ipv6.h                      |    1 -
 net/ipv6/netfilter/nf_conntrack_reasm.c |   13 ++++++++++++-
 net/ipv6/reassembly.c                   |    3 +--
 3 files changed, 13 insertions(+), 4 deletions(-)

include/net/ipv6.h +0 -1

index cbd768b..a7112da 100644
@@ -364,7 +364,6 @@ struct ip6_create_arg {
 	struct in6_addr *dst;
 };
 
-void ip6_frag_init(struct inet_frag_queue *q, void *a);
 
 static inline int ipv6_addr_any(const struct in6_addr *a)
 {

net/ipv6/netfilter/nf_conntrack_reasm.c +12 -1

index 66b6161..4a61d14 100644
@@ -146,6 +146,17 @@ static void nf_ct_frag6_evictor(void)
 	local_bh_enable();
 }
 
+static void nf_ct_queue_init(struct inet_frag_queue *q, void *a)
+{
+	struct nf_ct_frag6_queue *fq;
+	struct ip6_create_arg *arg = a;
+
+	fq = container_of(q, struct nf_ct_frag6_queue, q);
+	fq->id = arg->id;
+	ipv6_addr_copy(&fq->saddr, arg->src);
+	ipv6_addr_copy(&fq->daddr, arg->dst);
+}
+
 static int nf_ct_frag_match(struct inet_frag_queue *q, void *a)
 {
 	struct nf_ct_frag6_queue *fq;

@@ -672,7 +683,7 @@ void nf_ct_frag6_output(unsigned int hooknum, struct sk_buff *skb,
 int nf_ct_frag6_init(void)
 {
 	nf_frags.hashfn = nf_hashfn;
-	nf_frags.constructor = ip6_frag_init;
+	nf_frags.constructor = nf_ct_frag_init;
 	nf_frags.destructor = NULL;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);

net/ipv6/reassembly.c +1 -2

index 2fa4355..9f9b6a2 100644
@@ -157,7 +157,7 @@ static inline void frag_kfree_skb(struct netns_frags *nf,
 	kfree_skb(skb);
 }
 
-void ip6_frag_init(struct inet_frag_queue *q, void *a)
+static void ip6_frag_init(struct inet_frag_queue *q, void *a)
 {
 	struct frag_queue *fq = container_of(q, struct frag_queue, q);
 	struct ip6_create_arg *arg = a;

@@ -167,7 +167,6 @@ void ip6_frag_init(struct inet_frag_queue *q, void *a)
 	ipv6_addr_copy(&fq->saddr, arg->src);
 	ipv6_addr_copy(&fq->daddr, arg->dst);
 }
-EXPORT_SYMBOL(ip6_frag_init);
 
 /* Destruction primitives. */

Signature · 6 lines

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html