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

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

[Shan Wei]

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 <shanwei@cn.fujitsu.com>
---
 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(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index cbd768b..a7112da 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -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)
 {
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 66b6161..4a61d14 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -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);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 2fa4355..9f9b6a2 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -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. */
 
-- 
1.6.3.3

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

[David Miller]

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Tue, 26 Jan 2010 10:31:10 +0800

> 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 <shanwei@cn.fujitsu.com>

This breakage was recently introduced by:

commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d
Author: Patrick McHardy <kaber@trash.net>
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 <kaber@trash.net>

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

[Shan Wei]

David Miller wrote, at 01/26/2010 03:32 PM:
> From: Shan Wei <shanwei@cn.fujitsu.com>
> Date: Tue, 26 Jan 2010 10:31:10 +0800
> 
>> 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 <shanwei@cn.fujitsu.com>
> 
> This breakage was recently introduced by:
> 
> commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d
> Author: Patrick McHardy <kaber@trash.net>
> 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 <shanwei@cn.fujitsu.com>
---
 net/ipv6/netfilter/nf_conntrack_reasm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 312c20a..624a548 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -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;
 
-- 
1.6.3.3

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

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

[Patrick McHardy]

Shan Wei wrote:
> David Miller wrote, at 01/26/2010 03:32 PM:
>> From: Shan Wei <shanwei@cn.fujitsu.com>
>> Date: Tue, 26 Jan 2010 10:31:10 +0800
>>
>>> 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 <shanwei@cn.fujitsu.com>
>> This breakage was recently introduced by:
>>
>> commit 0b5ccb2ee250136dd7385b1c7da28417d0d4d32d
>> Author: Patrick McHardy <kaber@trash.net>
>> 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.

> 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 <shanwei@cn.fujitsu.com>

Acked-by: Patrick McHardy <kaber@trash.net>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 26 Jan 2010 13:46:46 +0100

> 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... :-)

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

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Tue, 26 Jan 2010 13:46:46 +0100
> 
>> 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.

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

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

Thanks :)

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

[Pascal Hambourg]

Hello,

David Miller a écrit :
> 
> 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.

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

[Yasuyuki KOZAKAI]

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 <shanwei@cn.fujitsu.com>
Date: Tue, 26 Jan 2010 10:31:10 +0800

> 
> 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 <shanwei@cn.fujitsu.com>
> ---
>  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(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index cbd768b..a7112da 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -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)
>  {
> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index 66b6161..4a61d14 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -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);
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 2fa4355..9f9b6a2 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -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. */
>  
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html