netns: oops in ip_frag_reasm incrementing stats

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] <[email protected]>

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] <[email protected]>

---
net/ipv4/ip_fragment.c | 14 +++++++-------

1 files changed, 7 insertions(+), 7 deletions(-)

net/ipv4/ip_fragment.c +0 -0

index 6659ac0..8f150d5 100644

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

Quoted 17 lines

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] <[email protected]>

---
net/ipv4/ip_fragment.c | 14 +++++++-------

1 files changed, 7 insertions(+), 7 deletions(-)

net/ipv4/ip_fragment.c +0 -0

index 6659ac0..8f150d5 100644

...

net/ipv4/ip_fragment.c static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev, · line 551

Quoted 11 lines

net/ipv4/ip_fragment.c +1 -1

-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;

net/ipv4/ip_fragment.c +1 -1

@@ -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);

Quoted 37 lines · Jarek Poplawski

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

Quoted 17 lines

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] <[email protected]>

---
net/ipv4/ip_fragment.c | 14 +++++++-------

1 files changed, 7 insertions(+), 7 deletions(-)

net/ipv4/ip_fragment.c +0 -0

index 6659ac0..8f150d5 100644

...

net/ipv4/ip_fragment.c static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev, · line 551

Quoted 11 lines

net/ipv4/ip_fragment.c +1 -1

-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;

net/ipv4/ip_fragment.c +1 -1

@@ -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?:

Quoted 6 lines

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?

Do you want to send a patch yourself or should I do it?

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] <[email protected]>

---

net/ipv4/ip_fragment.c |    5 +++--
net/ipv6/reassembly.c  |    7 +++----
2 files changed, 6 insertions(+), 6 deletions(-)

net/ipv4/ip_fragment.c +2 -1

index 6659ac0..4806e33 100644
@@ -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:

net/ipv4/ip_fragment.c +1 -1

-	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_REASMFAILS);
+	IP_INC_STATS_BH(net, IPSTATS_MIB_REASMFAILS);

	return err;
}

net/ipv6/reassembly.c +2 -2

index 3c57511..e9ac7a1 100644
@@ -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();

net/ipv6/reassembly.c +1 -2

-	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;
}

Signature · 1 line

1.5.6.5

Quoted 5 lines · 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.

Quoted 8 lines

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:

Review

Quoted 1 line

(quoted diff) +1 -0

+	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.

Quoted 17 lines · Jarek Poplawski

Quoted 5 lines · 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.

Quoted 8 lines

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

Quoted 7 lines

Anyway, ipv4 looks OK to me, but ipv6 looks like something is different:

Review

Quoted 1 line

(quoted diff) +1 -0

+	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.

Quoted 36 lines

Quoted 17 lines · Jarek Poplawski

Quoted 5 lines · 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.

Quoted 8 lines

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

Quoted 7 lines

Anyway, ipv4 looks OK to me, but ipv6 looks like something is different:

Review

Quoted 1 line

(quoted diff) +1 -0

+	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)"

Quoted 3 lines

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

From: Jarek Poplawski <[email protected]>
Date: Wed, 18 Mar 2009 07:26:58 +0000

Quoted 37 lines · On 17-03-2009 14:54, Jorge Boncompte [DTI2] wrote:

Quoted 36 lines

Quoted 17 lines · Jarek Poplawski

Quoted 5 lines · 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.

Quoted 8 lines

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

Quoted 7 lines

Anyway, ipv4 looks OK to me, but ipv6 looks like something is different:

Review

Quoted 1 line

(quoted diff) +1 -0

+	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)"

Quoted 8 lines

Quoted 3 lines

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

Quoted 4 lines

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.

From: Jarek Poplawski <[email protected]>
Date: Thu, 19 Mar 2009 22:54:28 +0100

Quoted 6 lines · David Miller

Quoted 4 lines

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.

Quoted 3 lines

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.