[PATCH] netns: oops in ip_frag_reasm incrementing stats

[PATCH] netns: oops in ip_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

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

skb->dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.

Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
conntrack reassembles them on the OUTPUT path you hit this code path.

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
  net/ipv4/ip_fragment.c |   14 +++++++-------
  1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 6659ac0..8f150d5 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -84,7 +84,7 @@ int ip_frag_mem(struct net *net)
  	return atomic_read(&net->ipv4.frags.mem);
  }

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct 
sk_buff *prev,
  			 struct net_device *dev);

  struct ip4_create_arg {
@@ -296,7 +296,7 @@ static int ip_frag_reinit(struct ipq *qp)
  }

  /* Add new segment to existing queue. */
-static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
+static int ip_frag_queue(struct net *net, struct ipq *qp, struct 
sk_buff *skb)
  {
  	struct sk_buff *prev, *next;
  	struct net_device *dev;
@@ -445,7 +445,7 @@ static int ip_frag_queue(struct ipq *qp, struct 
sk_buff *skb)

  	if (qp->q.last_in == (INET_FRAG_FIRST_IN | INET_FRAG_LAST_IN) &&
  	    qp->q.meat == qp->q.len)
-		return ip_frag_reasm(qp, prev, dev);
+		return ip_frag_reasm(net, qp, prev, dev);

  	write_lock(&ip4_frags.lock);
  	list_move_tail(&qp->q.lru_list, &qp->q.net->lru_list);
@@ -460,7 +460,7 @@ err:

  /* Build a new IP datagram from all its fragments. */

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct 
sk_buff *prev,
  			 struct net_device *dev)
  {
  	struct iphdr *iph;
@@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct 
sk_buff *prev,
  	iph = ip_hdr(head);
  	iph->frag_off = 0;
  	iph->tot_len = htons(len);
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
  	qp->q.fragments = NULL;
  	return 0;

@@ -562,7 +562,7 @@ out_oversize:
  		printk(KERN_INFO "Oversized IP packet from %pI4.\n",
  			&qp->saddr);
  out_fail:
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
  	return err;
  }

@@ -585,7 +585,7 @@ int ip_defrag(struct sk_buff *skb, u32 user)

  		spin_lock(&qp->q.lock);

-		ret = ip_frag_queue(qp, skb);
+		ret = ip_frag_queue(net, qp, skb);

  		spin_unlock(&qp->q.lock);
  		ipq_put(qp);
-- 
1.5.6.5


[-- Attachment #2: netns-fragmentation.patch --]
[-- Type: application/mbox, Size: 2450 bytes --]

[PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.

Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
conntrack reassembles them on the OUTPUT path you hit this code path.

Changes from v1:
	- Fixed description

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
  net/ipv4/ip_fragment.c |   14 +++++++-------
  1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 6659ac0..8f150d5 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -84,7 +84,7 @@ int ip_frag_mem(struct net *net)
  	return atomic_read(&net->ipv4.frags.mem);
  }

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct 
sk_buff *prev,
  			 struct net_device *dev);

  struct ip4_create_arg {
@@ -296,7 +296,7 @@ static int ip_frag_reinit(struct ipq *qp)
  }

  /* Add new segment to existing queue. */
-static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
+static int ip_frag_queue(struct net *net, struct ipq *qp, struct 
sk_buff *skb)
  {
  	struct sk_buff *prev, *next;
  	struct net_device *dev;
@@ -445,7 +445,7 @@ static int ip_frag_queue(struct ipq *qp, struct 
sk_buff *skb)

  	if (qp->q.last_in == (INET_FRAG_FIRST_IN | INET_FRAG_LAST_IN) &&
  	    qp->q.meat == qp->q.len)
-		return ip_frag_reasm(qp, prev, dev);
+		return ip_frag_reasm(net, qp, prev, dev);

  	write_lock(&ip4_frags.lock);
  	list_move_tail(&qp->q.lru_list, &qp->q.net->lru_list);
@@ -460,7 +460,7 @@ err:

  /* Build a new IP datagram from all its fragments. */

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct 
sk_buff *prev,
  			 struct net_device *dev)
  {
  	struct iphdr *iph;
@@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct 
sk_buff *prev,
  	iph = ip_hdr(head);
  	iph->frag_off = 0;
  	iph->tot_len = htons(len);
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
  	qp->q.fragments = NULL;
  	return 0;

@@ -562,7 +562,7 @@ out_oversize:
  		printk(KERN_INFO "Oversized IP packet from %pI4.\n",
  			&qp->saddr);
  out_fail:
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
  	return err;
  }

@@ -585,7 +585,7 @@ int ip_defrag(struct sk_buff *skb, u32 user)

  		spin_lock(&qp->q.lock);

-		ret = ip_frag_queue(qp, skb);
+		ret = ip_frag_queue(net, qp, skb);

  		spin_unlock(&qp->q.lock);
  		ipq_put(qp);
-- 
1.5.6.5

Re: [PATCH] netns: oops in ip_frag_reasm incrementing stats

[David Miller]

From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Date: Fri, 13 Mar 2009 17:21:08 +0100

> skb->dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.
> 
> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> conntrack reassembles them on the OUTPUT path you hit this code path.
> 
> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>

Your patch was corrupted by your email client, please fix this up and
resubmit.

[PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.

Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
conntrack reassembles them on the OUTPUT path you hit this code path.

Changes from v1:
   - Fixed description

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
net/ipv4/ip_fragment.c |   14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 6659ac0..8f150d5 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -84,7 +84,7 @@ int ip_frag_mem(struct net *net)
	return atomic_read(&net->ipv4.frags.mem);
}

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct sk_buff *prev,
			 struct net_device *dev);

struct ip4_create_arg {
@@ -296,7 +296,7 @@ static int ip_frag_reinit(struct ipq *qp)
}

/* Add new segment to existing queue. */
-static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
+static int ip_frag_queue(struct net *net, struct ipq *qp, struct sk_buff *skb)
{
	struct sk_buff *prev, *next;
	struct net_device *dev;
@@ -445,7 +445,7 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)

	if (qp->q.last_in == (INET_FRAG_FIRST_IN | INET_FRAG_LAST_IN) &&
	    qp->q.meat == qp->q.len)
-		return ip_frag_reasm(qp, prev, dev);
+		return ip_frag_reasm(net, qp, prev, dev);

	write_lock(&ip4_frags.lock);
	list_move_tail(&qp->q.lru_list, &qp->q.net->lru_list);
@@ -460,7 +460,7 @@ err:

/* Build a new IP datagram from all its fragments. */

-static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
+static int ip_frag_reasm(struct net *net, struct ipq *qp, struct sk_buff *prev,
			 struct net_device *dev)
{
	struct iphdr *iph;
@@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
	iph = ip_hdr(head);
	iph->frag_off = 0;
	iph->tot_len = htons(len);
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
	qp->q.fragments = NULL;
	return 0;

@@ -562,7 +562,7 @@ out_oversize:
		printk(KERN_INFO "Oversized IP packet from %pI4.\n",
			&qp->saddr);
out_fail:
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
	return err;
}

@@ -585,7 +585,7 @@ int ip_defrag(struct sk_buff *skb, u32 user)

		spin_lock(&qp->q.lock);

-		ret = ip_frag_queue(qp, skb);
+		ret = ip_frag_queue(net, qp, skb);

		spin_unlock(&qp->q.lock);
		ipq_put(qp);
-- 
1.5.6.5

Re: [PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jarek Poplawski]

Jorge Boncompte [DTI2] wrote, On 03/16/2009 01:09 PM:

> dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.
> 
> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> conntrack reassembles them on the OUTPUT path you hit this code path.
> 
> Changes from v1:
>    - Fixed description
> 
> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
> ---
> net/ipv4/ip_fragment.c |   14 +++++++-------
> 1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index 6659ac0..8f150d5 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c

...

> -static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
> +static int ip_frag_reasm(struct net *net, struct ipq *qp, struct sk_buff *prev,
> 			 struct net_device *dev)
> {
> 	struct iphdr *iph;
> @@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
> 	iph = ip_hdr(head);
> 	iph->frag_off = 0;
> 	iph->tot_len = htons(len);
> -	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
> +	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);


I didn't check this but isn't something like this possible here too?:

static inline int ip_frag_too_far(struct ipq *qp)
{
	...
        net = container_of(qp->q.net, struct net, ipv4.frags);
        IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);

Jarek P.

Re: [PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

Jarek Poplawski escribió:
> Jorge Boncompte [DTI2] wrote, On 03/16/2009 01:09 PM:
> 
>> dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.
>>
>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
>> conntrack reassembles them on the OUTPUT path you hit this code path.
>>
>> Changes from v1:
>>    - Fixed description
>>
>> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
>> ---
>> net/ipv4/ip_fragment.c |   14 +++++++-------
>> 1 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
>> index 6659ac0..8f150d5 100644
>> --- a/net/ipv4/ip_fragment.c
>> +++ b/net/ipv4/ip_fragment.c
> 
> ...
> 
>> -static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
>> +static int ip_frag_reasm(struct net *net, struct ipq *qp, struct sk_buff *prev,
>> 			 struct net_device *dev)
>> {
>> 	struct iphdr *iph;
>> @@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
>> 	iph = ip_hdr(head);
>> 	iph->frag_off = 0;
>> 	iph->tot_len = htons(len);
>> -	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
>> +	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
> 
> 
> I didn't check this but isn't something like this possible here too?:
> 
> static inline int ip_frag_too_far(struct ipq *qp)
> {
> 	...
>         net = container_of(qp->q.net, struct net, ipv4.frags);
>         IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
> 

	Yes, it seems so. I did not noticed how the rest of the code accessed
the net pointer, sorry.
	Do you want to send a patch yourself or should I do it?

	Regards,

	Jorge
-- 
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

Re: [PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jarek Poplawski]

On Mon, Mar 16, 2009 at 10:53:14PM +0100, Jorge Boncompte [DTI2] wrote:
> Jarek Poplawski escribió:
>> Jorge Boncompte [DTI2] wrote, On 03/16/2009 01:09 PM:
>>
>>> dev can be NULL on ip_frag_reasm for skb's coming from RAW sockets.
>>>
>>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
>>> conntrack reassembles them on the OUTPUT path you hit this code path.
>>>
>>> Changes from v1:
>>>    - Fixed description
>>>
>>> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
>>> ---
>>> net/ipv4/ip_fragment.c |   14 +++++++-------
>>> 1 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
>>> index 6659ac0..8f150d5 100644
>>> --- a/net/ipv4/ip_fragment.c
>>> +++ b/net/ipv4/ip_fragment.c
>>
>> ...
>>
>>> -static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
>>> +static int ip_frag_reasm(struct net *net, struct ipq *qp, struct sk_buff *prev,
>>> 			 struct net_device *dev)
>>> {
>>> 	struct iphdr *iph;
>>> @@ -548,7 +548,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
>>> 	iph = ip_hdr(head);
>>> 	iph->frag_off = 0;
>>> 	iph->tot_len = htons(len);
>>> -	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
>>> +	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
>>
>>
>> I didn't check this but isn't something like this possible here too?:
>>
>> static inline int ip_frag_too_far(struct ipq *qp)
>> {
>> 	...
>>         net = container_of(qp->q.net, struct net, ipv4.frags);
>>         IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
>>
>
> 	Yes, it seems so. I did not noticed how the rest of the code accessed
> the net pointer, sorry.
> 	Do you want to send a patch yourself or should I do it?
>

You did the whole work so I hope you'll finish this (after checking
and maybe some testing)!

Regards,
Jarek P.

Re: [PATCHv2] netns: oops in ip_frag_reasm incrementing stats

[Jarek Poplawski]

On Mon, Mar 16, 2009 at 10:53:14PM +0100, Jorge Boncompte [DTI2] wrote:
...
> 	Do you want to send a patch yourself or should I do it?

BTW, it seems you're expected to this for ipv6 too. ;-)

Jarek P.

[PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.

Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
conntrack reassembles them on the OUTPUT path you hit this code path.

You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"

Changes from v2: (address comments from Jarek Poplawski)
	- Patch reworked to get the net pointer with container_of()
	  instead of passing it to function calls.
	- Fix IPv6 code
Changes from v1:
	- Fixed description

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
 net/ipv4/ip_fragment.c |    5 +++--
 net/ipv6/reassembly.c  |    7 +++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 6659ac0..4806e33 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -463,6 +463,7 @@ err:
 static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
 			 struct net_device *dev)
 {
+	struct net *net = container_of(qp->q.net, struct net, ipv4.frags);
 	struct iphdr *iph;
 	struct sk_buff *fp, *head = qp->q.fragments;
 	int len;
@@ -548,7 +549,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
 	iph = ip_hdr(head);
 	iph->frag_off = 0;
 	iph->tot_len = htons(len);
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
 	qp->q.fragments = NULL;
 	return 0;
 
@@ -562,7 +563,7 @@ out_oversize:
 		printk(KERN_INFO "Oversized IP packet from %pI4.\n",
 			&qp->saddr);
 out_fail:
-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
 	return err;
 }
 
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 3c57511..e9ac7a1 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -452,6 +452,7 @@ err:
 static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
 			  struct net_device *dev)
 {
+	struct net *net = container_of(fq->q.net, struct net, ipv6.frags);
 	struct sk_buff *fp, *head = fq->q.fragments;
 	int    payload_len;
 	unsigned int nhoff;
@@ -551,8 +552,7 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
 					  head->csum);
 
 	rcu_read_lock();
-	IP6_INC_STATS_BH(dev_net(dev),
-			 __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
+	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
 	rcu_read_unlock();
 	fq->q.fragments = NULL;
 	return 1;
@@ -566,8 +566,7 @@ out_oom:
 		printk(KERN_DEBUG "ip6_frag_reasm: no memory for reassembly\n");
 out_fail:
 	rcu_read_lock();
-	IP6_INC_STATS_BH(dev_net(dev),
-			 __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
+	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
 	rcu_read_unlock();
 	return -1;
 }
-- 
1.5.6.5

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jarek Poplawski]

On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
>
> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> conntrack reassembles them on the OUTPUT path you hit this code path.
>
> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
>
> Changes from v2: (address comments from Jarek Poplawski)
> 	- Patch reworked to get the net pointer with container_of()
> 	  instead of passing it to function calls.
> 	- Fix IPv6 code
> Changes from v1:
> 	- Fixed description

I guess David will be interested only with the final state of changes,
so v1 & v2 are not necessary here...

Anyway, ipv4 looks OK to me, but ipv6 looks like something is
different:
> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);

It still depends on dev != NULL in __in6_dev_get(). I see there
is also used skb->dst for similar things in ip6_frag_queue(), so I
don't know: it needs rethinking, and maybe these patches should be
separated if you prefer.

Thanks,
Jarek P.

>
> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
> ---
> net/ipv4/ip_fragment.c |    5 +++--
> net/ipv6/reassembly.c  |    7 +++----
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index 6659ac0..4806e33 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -463,6 +463,7 @@ err:
> static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
> 			 struct net_device *dev)
> {
> +	struct net *net = container_of(qp->q.net, struct net, ipv4.frags);
> 	struct iphdr *iph;
> 	struct sk_buff *fp, *head = qp->q.fragments;
> 	int len;
> @@ -548,7 +549,7 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
> 	iph = ip_hdr(head);
> 	iph->frag_off = 0;
> 	iph->tot_len = htons(len);
> -	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMOKS);
> +	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMOKS);
> 	qp->q.fragments = NULL;
> 	return 0;
>
> @@ -562,7 +563,7 @@ out_oversize:
> 		printk(KERN_INFO "Oversized IP packet from %pI4.\n",
> 			&qp->saddr);
> out_fail:
> -	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
> +	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);
> 	return err;
> }
>
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 3c57511..e9ac7a1 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -452,6 +452,7 @@ err:
> static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> 			  struct net_device *dev)
> {
> +	struct net *net = container_of(fq->q.net, struct net, ipv6.frags);
> 	struct sk_buff *fp, *head = fq->q.fragments;
> 	int    payload_len;
> 	unsigned int nhoff;
> @@ -551,8 +552,7 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *prev,
> 					  head->csum);
>
> 	rcu_read_lock();
> -	IP6_INC_STATS_BH(dev_net(dev),
> -			 __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
> 	rcu_read_unlock();
> 	fq->q.fragments = NULL;
> 	return 1;
> @@ -566,8 +566,7 @@ out_oom:
> 		printk(KERN_DEBUG "ip6_frag_reasm: no memory for reassembly\n");
> out_fail:
> 	rcu_read_lock();
> -	IP6_INC_STATS_BH(dev_net(dev),
> -			 __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> 	rcu_read_unlock();
> 	return -1;
> }
> -- 
> 1.5.6.5
>

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jorge Boncompte [DTI2]]

Jarek Poplawski escribió:
> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
>>
>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
>> conntrack reassembles them on the OUTPUT path you hit this code path.
>>
>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
>>
>> Changes from v2: (address comments from Jarek Poplawski)
>> 	- Patch reworked to get the net pointer with container_of()
>> 	  instead of passing it to function calls.
>> 	- Fix IPv6 code
>> Changes from v1:
>> 	- Fixed description
> 
> I guess David will be interested only with the final state of changes,
> so v1 & v2 are not necessary here...
> 
> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
> different:
>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> 
> It still depends on dev != NULL in __in6_dev_get(). I see there
> is also used skb->dst for similar things in ip6_frag_queue(), so I
> don't know: it needs rethinking, and maybe these patches should be
> separated if you prefer.

	Not my day! :-) I should not look at code at 2 am and write patches
the day after, I confused _idev and idev in the check for != NULL in _DEVINC.

	I think this bug was first introduced by patch "[IPV6]: Per-interface
statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.

	If someone with more knowledge could confirm that using something like...

"(skb->dev ? skb->dev : skb->dst->dev)"

... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
I can test this.

	Regards,

	Jorge
-- 
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jarek Poplawski]

On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:
> Jarek Poplawski escribió:
>> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
>>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
>>>
>>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
>>> conntrack reassembles them on the OUTPUT path you hit this code path.
>>>
>>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
>>>
>>> Changes from v2: (address comments from Jarek Poplawski)
>>> 	- Patch reworked to get the net pointer with container_of()
>>> 	  instead of passing it to function calls.
>>> 	- Fix IPv6 code
>>> Changes from v1:
>>> 	- Fixed description
>> I guess David will be interested only with the final state of changes,
>> so v1 & v2 are not necessary here...
>>
>> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
>> different:
>>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
>> It still depends on dev != NULL in __in6_dev_get(). I see there
>> is also used skb->dst for similar things in ip6_frag_queue(), so I
>> don't know: it needs rethinking, and maybe these patches should be
>> separated if you prefer.
> 
> 	Not my day! :-) I should not look at code at 2 am and write patches
> the day after, I confused _idev and idev in the check for != NULL in _DEVINC.
> 
> 	I think this bug was first introduced by patch "[IPV6]: Per-interface
> statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.
> 
> 	If someone with more knowledge could confirm that using something like...
> 
> "(skb->dev ? skb->dev : skb->dst->dev)"
> 
> ... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
> I can test this.
> 

skb->dst->dev is used in ip6_frag_queue anyway, so it shouldn't be
worse if you do this similarly in ip6_frag_reasm. If you send it as
a separate patch, and write it's not tested, David will decide if he
wants it. Otherwise you can resend this ipv4 patch only.

Jarek P.

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 18 Mar 2009 07:26:58 +0000

> On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:
> > Jarek Poplawski escribió:
> >> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
> >>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
> >>>
> >>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> >>> conntrack reassembles them on the OUTPUT path you hit this code path.
> >>>
> >>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
> >>>
> >>> Changes from v2: (address comments from Jarek Poplawski)
> >>> 	- Patch reworked to get the net pointer with container_of()
> >>> 	  instead of passing it to function calls.
> >>> 	- Fix IPv6 code
> >>> Changes from v1:
> >>> 	- Fixed description
> >> I guess David will be interested only with the final state of changes,
> >> so v1 & v2 are not necessary here...
> >>
> >> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
> >> different:
> >>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> >> It still depends on dev != NULL in __in6_dev_get(). I see there
> >> is also used skb->dst for similar things in ip6_frag_queue(), so I
> >> don't know: it needs rethinking, and maybe these patches should be
> >> separated if you prefer.
> > 
> > 	Not my day! :-) I should not look at code at 2 am and write patches
> > the day after, I confused _idev and idev in the check for != NULL in _DEVINC.
> > 
> > 	I think this bug was first introduced by patch "[IPV6]: Per-interface
> > statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.
> > 
> > 	If someone with more knowledge could confirm that using something like...
> > 
> > "(skb->dev ? skb->dev : skb->dst->dev)"
> > 
> > ... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
> > I can test this.
> > 
> 
> skb->dst->dev is used in ip6_frag_queue anyway, so it shouldn't be
> worse if you do this similarly in ip6_frag_reasm. If you send it as
> a separate patch, and write it's not tested, David will decide if he
> wants it. Otherwise you can resend this ipv4 patch only.

Netfilter on ipv6 handles reassembly differently and in a way
that won't result in dev being NULL here.

I've applied Jorge's patch, thanks everyone.

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jarek Poplawski]

On Wed, Mar 18, 2009 at 11:26:51PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 18 Mar 2009 07:26:58 +0000
> 
> > On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:
> > > Jarek Poplawski escribió:
> > >> On Tue, Mar 17, 2009 at 12:55:42PM +0100, Jorge Boncompte [DTI2] wrote:
> > >>> dev can be NULL in ip[6]_frag_reasm for skb's coming from RAW sockets.
> > >>>
> > >>> Quagga's OSPFD sends fragmented packets on a RAW socket, when netfilter
> > >>> conntrack reassembles them on the OUTPUT path you hit this code path.
> > >>>
> > >>> You can test it with something like "hping2 -0 -d 2000 -f AA.BB.CC.DD"
> > >>>
> > >>> Changes from v2: (address comments from Jarek Poplawski)
> > >>> 	- Patch reworked to get the net pointer with container_of()
> > >>> 	  instead of passing it to function calls.
> > >>> 	- Fix IPv6 code
> > >>> Changes from v1:
> > >>> 	- Fixed description
> > >> I guess David will be interested only with the final state of changes,
> > >> so v1 & v2 are not necessary here...
> > >>
> > >> Anyway, ipv4 looks OK to me, but ipv6 looks like something is
> > >> different:
> > >>> +	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
> > >> It still depends on dev != NULL in __in6_dev_get(). I see there
> > >> is also used skb->dst for similar things in ip6_frag_queue(), so I
> > >> don't know: it needs rethinking, and maybe these patches should be
> > >> separated if you prefer.
> > > 
> > > 	Not my day! :-) I should not look at code at 2 am and write patches
> > > the day after, I confused _idev and idev in the check for != NULL in _DEVINC.
> > > 
> > > 	I think this bug was first introduced by patch "[IPV6]: Per-interface
> > > statistics support." from YOSHIFUJI Hideaki on Nov 4, 2006.
> > > 
> > > 	If someone with more knowledge could confirm that using something like...
> > > 
> > > "(skb->dev ? skb->dev : skb->dst->dev)"
> > > 
> > > ... here is fine I'll redo this part and resend. I do not have an IPv6 setup where
> > > I can test this.
> > > 
> > 
> > skb->dst->dev is used in ip6_frag_queue anyway, so it shouldn't be
> > worse if you do this similarly in ip6_frag_reasm. If you send it as
> > a separate patch, and write it's not tested, David will decide if he
> > wants it. Otherwise you can resend this ipv4 patch only.
> 
> Netfilter on ipv6 handles reassembly differently and in a way
> that won't result in dev being NULL here.
> 
> I've applied Jorge's patch, thanks everyone.

My proposal is to revert the ipv6/reassembly part of this patch yet;
it doesn't fix anything, and the changelog is only misleading if it's
like you said.

Jarek P.

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 19 Mar 2009 22:54:28 +0100

> On Wed, Mar 18, 2009 at 11:26:51PM -0700, David Miller wrote:
> > Netfilter on ipv6 handles reassembly differently and in a way
> > that won't result in dev being NULL here.
> > 
> > I've applied Jorge's patch, thanks everyone.
> 
> My proposal is to revert the ipv6/reassembly part of this patch yet;
> it doesn't fix anything, and the changelog is only misleading if it's
> like you said.

I've already pushed the change out to net-2.6 so I'd really prefer not
to do that.  Make your desires known more emphatically before a patch
has been sitting around for days.

At worst it's a consistency cleanup with an inaccurate commit message,
reverting will only make the situation more confusing and ugly.

Re: [PATCHv3] netns: oops in ip[6]_frag_reasm incrementing stats

[Jarek Poplawski]

On Thu, Mar 19, 2009 at 02:56:49PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Thu, 19 Mar 2009 22:54:28 +0100
> 
> > On Wed, Mar 18, 2009 at 11:26:51PM -0700, David Miller wrote:
> > > Netfilter on ipv6 handles reassembly differently and in a way
> > > that won't result in dev being NULL here.
> > > 
> > > I've applied Jorge's patch, thanks everyone.
> > 
> > My proposal is to revert the ipv6/reassembly part of this patch yet;
> > it doesn't fix anything, and the changelog is only misleading if it's
> > like you said.
> 
> I've already pushed the change out to net-2.6 so I'd really prefer not
> to do that.  Make your desires known more emphatically before a patch
> has been sitting around for days.

I guess you should re-read the thread: if not me, the author of this
patch was very emphatical...

> 
> At worst it's a consistency cleanup with an inaccurate commit message,
> reverting will only make the situation more confusing and ugly.

Sure, no big deal.

Jarek P.