2.6.25-rc: Null dereference in ip_defrag

2.6.25-rc: Null dereference in ip_defrag

[Phil Oester]

Been seeing occasional panics in my testing of 2.6.25-rc in ip_defrag.
Offending line in ip_defrag is here:

	net = skb->dev->nd_net

where dev is NULL.  Bisected the problem down to commit
ac18e7509e7df327e30d6e073a787d922eaf211d ([NETNS][FRAGS]: Make the
inet_frag_queue lookup work in namespaces).  

To prevent panic, I added the below patch (whitespace damaged):

--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -568,6 +568,14 @@ int ip_defrag(struct sk_buff *skb, u32 user)
 
        IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
+       if (!skb->dev) {
+               printk("ip_defrag_bug: %u.%u.%u.%u -> %u.%u.%u.%u\n",
+                      NIPQUAD(ip_hdr(skb)->saddr), NIPQUAD(ip_hdr(skb)->daddr));
+               WARN_ON(1);
+               kfree_skb(skb);
+               return -ENOMEM;
+       }
+

And the packets causing the problem are all multicast fragments being
generated by Quagga's OSPFD (see debug output below).  Tried manually generating
some multicast fragments with iperf, but couldn't reproduce it.  

Any ideas?

Phil


 ip_defrag_bug: 10.253.13.122 -> 224.0.0.5
 ------------[ cut here ]------------
 WARNING: at net/ipv4/ip_fragment.c:574 ip_defrag+0x9d/0xa0d()
 Pid: 1662, comm: ospfd Not tainted 2.6.25-rc4 #4
 
 Call Trace:
  [<ffffffff8022742a>] warn_on_slowpath+0x53/0x66
  [<ffffffff80228230>] ? printk+0x67/0x69
  [<ffffffff803b36f5>] ? skb_release_data+0xa8/0xad
  [<ffffffff803b35b3>] ? __kfree_skb+0x74/0x78
  [<ffffffff803d77c7>] ip_defrag+0x9d/0xa0d
  [<ffffffff803b15c6>] ? sock_def_write_space+0x18/0x89
  [<ffffffff80404324>] ipv4_conntrack_defrag+0x67/0x96
  [<ffffffff803caa7b>] nf_iterate+0x41/0x81
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803cab19>] nf_hook_slow+0x5e/0xbe
  [<ffffffff803f29b0>] ? dst_output+0x0/0x10
  [<ffffffff803f3b6e>] raw_sendmsg+0x586/0x758
  [<ffffffff803fb169>] inet_sendmsg+0x46/0x53
  [<ffffffff803adef9>] sock_sendmsg+0xdf/0xf8
  [<ffffffff80421cd1>] ? _spin_lock_bh+0x11/0x29
  [<ffffffff803afbfc>] ? release_sock+0x9b/0xa3
  [<ffffffff80238b37>] ? autoremove_wake_function+0x0/0x38
  [<ffffffff803ad359>] ? move_addr_to_kernel+0x25/0x35
  [<ffffffff803c4c6f>] ? verify_compat_iovec+0x60/0x9e
  [<ffffffff803ae0f3>] sys_sendmsg+0x1e1/0x253
  [<ffffffff802332ee>] ? getrusage+0x1c9/0x1e6
  [<ffffffff804206d4>] ? thread_return+0x3d/0x9c
  [<ffffffff803c45d0>] compat_sys_sendmsg+0xf/0x11
  [<ffffffff803c4e82>] compat_sys_socketcall+0x13f/0x158
  [<ffffffff8021cc12>] sysenter_do_call+0x1b/0x66
 
 ---[ end trace 48218d00aa061d3c ]---

Re: 2.6.25-rc: Null dereference in ip_defrag

[Pavel Emelyanov]

Phil Oester wrote:
> Been seeing occasional panics in my testing of 2.6.25-rc in ip_defrag.
> Offending line in ip_defrag is here:
> 
> 	net = skb->dev->nd_net

Yikes :(

> where dev is NULL.  Bisected the problem down to commit
> ac18e7509e7df327e30d6e073a787d922eaf211d ([NETNS][FRAGS]: Make the
> inet_frag_queue lookup work in namespaces).  
> 
> To prevent panic, I added the below patch (whitespace damaged):
> 
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -568,6 +568,14 @@ int ip_defrag(struct sk_buff *skb, u32 user)
>  
>         IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
>  
> +       if (!skb->dev) {
> +               printk("ip_defrag_bug: %u.%u.%u.%u -> %u.%u.%u.%u\n",
> +                      NIPQUAD(ip_hdr(skb)->saddr), NIPQUAD(ip_hdr(skb)->daddr));
> +               WARN_ON(1);
> +               kfree_skb(skb);
> +               return -ENOMEM;
> +       }
> +
> 
> And the packets causing the problem are all multicast fragments being
> generated by Quagga's OSPFD (see debug output below).  Tried manually generating
> some multicast fragments with iperf, but couldn't reproduce it.  
> 
> Any ideas?

This is the same as the problem fixed here:

    commit 4136cd523eb0c0bd53173e16fd7406d31d05824f
    [IPV4]: route: fix crash ip_route_input

The sk_buff does not have a valid dev sometimes in ip_defrag() :(
and you have to setup conntrack rules to make packets go this way.
But unlike the above problem we cannot even trust the skb->dst to
be not NULL...

Can you check with this patch, please (untested, but should work)?

diff --git a/include/net/ip.h b/include/net/ip.h
index 9f50d4f..fc6b650 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -330,7 +330,8 @@ enum ip_defrag_users
 	IP_DEFRAG_VS_FWD
 };
 
-int ip_defrag(struct sk_buff *skb, u32 user);
+struct net;
+int ip_defrag(struct net *net, struct sk_buff *skb, u32 user);
 int ip_frag_mem(struct net *net);
 int ip_frag_nqueues(struct net *net);
 
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index a2e92f9..aad6652 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -561,14 +561,12 @@ out_fail:
 }
 
 /* Process an incoming IP datagram fragment. */
-int ip_defrag(struct sk_buff *skb, u32 user)
+int ip_defrag(struct net *net, struct sk_buff *skb, u32 user)
 {
 	struct ipq *qp;
-	struct net *net;
 
 	IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
-	net = skb->dev->nd_net;
 	/* Start by cleaning up the memory. */
 	if (atomic_read(&net->ipv4.frags.mem) > net->ipv4.frags.high_thresh)
 		ip_evictor(net);
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 6563139..25c6846 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -172,7 +172,8 @@ int ip_call_ra_chain(struct sk_buff *skb)
 		    (!sk->sk_bound_dev_if ||
 		     sk->sk_bound_dev_if == skb->dev->ifindex)) {
 			if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-				if (ip_defrag(skb, IP_DEFRAG_CALL_RA_CHAIN)) {
+				if (ip_defrag(skb->dev->nd_net, skb,
+						IP_DEFRAG_CALL_RA_CHAIN)) {
 					read_unlock(&ip_ra_lock);
 					return 1;
 				}
@@ -256,7 +257,7 @@ int ip_local_deliver(struct sk_buff *skb)
 	 */
 
 	if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-		if (ip_defrag(skb, IP_DEFRAG_LOCAL_DELIVER))
+		if (ip_defrag(skb->dev->nd_net, skb, IP_DEFRAG_LOCAL_DELIVER))
 			return 0;
 	}
 
diff --git a/net/ipv4/ipvs/ip_vs_core.c b/net/ipv4/ipvs/ip_vs_core.c
index 963981a..d2c3c0f 100644
--- a/net/ipv4/ipvs/ip_vs_core.c
+++ b/net/ipv4/ipvs/ip_vs_core.c
@@ -506,7 +506,7 @@ __sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset)
 
 static inline int ip_vs_gather_frags(struct sk_buff *skb, u_int32_t user)
 {
-	int err = ip_defrag(skb, user);
+	int err = ip_defrag(&init_net, skb, user);
 
 	if (!err)
 		ip_send_check(ip_hdr(skb));
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index a65b845..00a0f63 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -58,14 +58,15 @@ static int ipv4_print_tuple(struct seq_file *s,
 }
 
 /* Returns new sk_buff, or NULL */
-static int nf_ct_ipv4_gather_frags(struct sk_buff *skb, u_int32_t user)
+static int nf_ct_ipv4_gather_frags(struct net *net, struct sk_buff *skb,
+		u_int32_t user)
 {
 	int err;
 
 	skb_orphan(skb);
 
 	local_bh_disable();
-	err = ip_defrag(skb, user);
+	err = ip_defrag(net, skb, user);
 	local_bh_enable();
 
 	if (!err)
@@ -138,6 +139,9 @@ static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
 					  const struct net_device *out,
 					  int (*okfn)(struct sk_buff *))
 {
+	u_int32_t user;
+	struct net *net;
+
 	/* Previously seen (loopback)?  Ignore.  Do this before
 	   fragment check. */
 	if (skb->nfct)
@@ -145,10 +149,14 @@ static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
 
 	/* Gather fragments. */
 	if (ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)) {
-		if (nf_ct_ipv4_gather_frags(skb,
-					    hooknum == NF_INET_PRE_ROUTING ?
-					    IP_DEFRAG_CONNTRACK_IN :
-					    IP_DEFRAG_CONNTRACK_OUT))
+		if (hooknum == NF_INET_PRE_ROUTING) {
+			user = IP_DEFRAG_CONNTRACK_IN;
+			net = in->nd_net;
+		} else {
+			user = IP_DEFRAG_CONNTRACK_OUT;
+			net = out->nd_net;
+		}
+		if (nf_ct_ipv4_gather_frags(net, skb, user))
 			return NF_STOLEN;
 	}
 	return NF_ACCEPT;



> Phil
> 
> 
>  ip_defrag_bug: 10.253.13.122 -> 224.0.0.5
>  ------------[ cut here ]------------
>  WARNING: at net/ipv4/ip_fragment.c:574 ip_defrag+0x9d/0xa0d()
>  Pid: 1662, comm: ospfd Not tainted 2.6.25-rc4 #4
>  
>  Call Trace:
>   [<ffffffff8022742a>] warn_on_slowpath+0x53/0x66
>   [<ffffffff80228230>] ? printk+0x67/0x69
>   [<ffffffff803b36f5>] ? skb_release_data+0xa8/0xad
>   [<ffffffff803b35b3>] ? __kfree_skb+0x74/0x78
>   [<ffffffff803d77c7>] ip_defrag+0x9d/0xa0d
>   [<ffffffff803b15c6>] ? sock_def_write_space+0x18/0x89
>   [<ffffffff80404324>] ipv4_conntrack_defrag+0x67/0x96
>   [<ffffffff803caa7b>] nf_iterate+0x41/0x81
>   [<ffffffff803f29b0>] ? dst_output+0x0/0x10
>   [<ffffffff803cab19>] nf_hook_slow+0x5e/0xbe
>   [<ffffffff803f29b0>] ? dst_output+0x0/0x10
>   [<ffffffff803f3b6e>] raw_sendmsg+0x586/0x758
>   [<ffffffff803fb169>] inet_sendmsg+0x46/0x53
>   [<ffffffff803adef9>] sock_sendmsg+0xdf/0xf8
>   [<ffffffff80421cd1>] ? _spin_lock_bh+0x11/0x29
>   [<ffffffff803afbfc>] ? release_sock+0x9b/0xa3
>   [<ffffffff80238b37>] ? autoremove_wake_function+0x0/0x38
>   [<ffffffff803ad359>] ? move_addr_to_kernel+0x25/0x35
>   [<ffffffff803c4c6f>] ? verify_compat_iovec+0x60/0x9e
>   [<ffffffff803ae0f3>] sys_sendmsg+0x1e1/0x253
>   [<ffffffff802332ee>] ? getrusage+0x1c9/0x1e6
>   [<ffffffff804206d4>] ? thread_return+0x3d/0x9c
>   [<ffffffff803c45d0>] compat_sys_sendmsg+0xf/0x11
>   [<ffffffff803c4e82>] compat_sys_socketcall+0x13f/0x158
>   [<ffffffff8021cc12>] sysenter_do_call+0x1b/0x66
>  
>  ---[ end trace 48218d00aa061d3c ]---
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: 2.6.25-rc: Null dereference in ip_defrag

[Patrick McHardy]

Pavel Emelyanov wrote:
> Phil Oester wrote:
>> And the packets causing the problem are all multicast fragments being
>> generated by Quagga's OSPFD (see debug output below).  Tried manually generating
>> some multicast fragments with iperf, but couldn't reproduce it.  
>>
>> Any ideas?
> 
> This is the same as the problem fixed here:
> 
>     commit 4136cd523eb0c0bd53173e16fd7406d31d05824f
>     [IPV4]: route: fix crash ip_route_input
> 
> The sk_buff does not have a valid dev sometimes in ip_defrag() :(
> and you have to setup conntrack rules to make packets go this way.
> But unlike the above problem we cannot even trust the skb->dst to
> be not NULL...


We can on output. Usually we don't even see fragments in conntrack
on output since we've defer fragmentation until all netfilter hooks
have been processed. Quagga is generating fragments using raw sockets
and IP_HDRINCL though.

 > Can you check with this patch, please (untested, but should work)?

This is getting pretty ugly. Shouldn't

int ip_defrag(struct sk_buff *skb, u32 user)
{
...
-         net = skb->dev->nd_net;
+         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;

work as well?

Re: 2.6.25-rc: Null dereference in ip_defrag

[Denis V. Lunev]

On Mon, 2008-03-17 at 18:43 +0100, Patrick McHardy wrote:
> Pavel Emelyanov wrote:
> > Phil Oester wrote:
> >> And the packets causing the problem are all multicast fragments being
> >> generated by Quagga's OSPFD (see debug output below).  Tried manually generating
> >> some multicast fragments with iperf, but couldn't reproduce it.  
> >>
> >> Any ideas?
> > 
> > This is the same as the problem fixed here:
> > 
> >     commit 4136cd523eb0c0bd53173e16fd7406d31d05824f
> >     [IPV4]: route: fix crash ip_route_input
> > 
> > The sk_buff does not have a valid dev sometimes in ip_defrag() :(
> > and you have to setup conntrack rules to make packets go this way.
> > But unlike the above problem we cannot even trust the skb->dst to
> > be not NULL...
> 
> 
> We can on output. Usually we don't even see fragments in conntrack
> on output since we've defer fragmentation until all netfilter hooks
> have been processed. Quagga is generating fragments using raw sockets
> and IP_HDRINCL though.
> 
>  > Can you check with this patch, please (untested, but should work)?
> 
> This is getting pretty ugly. Shouldn't
> 
> int ip_defrag(struct sk_buff *skb, u32 user)
> {
> ...
> -         net = skb->dev->nd_net;
> +         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
>From my POW the we can just get skb->dst->dev. Could we?

I think that on IP level dealing with fragments we always have the
destination entry. I'll recheck this tomorrow.

Re: 2.6.25-rc: Null dereference in ip_defrag

[Patrick McHardy]

Denis V. Lunev wrote:
> On Mon, 2008-03-17 at 18:43 +0100, Patrick McHardy wrote:
>> Pavel Emelyanov wrote:
>>
>>  > Can you check with this patch, please (untested, but should work)?
>>
>> This is getting pretty ugly. Shouldn't
>>
>> int ip_defrag(struct sk_buff *skb, u32 user)
>> {
>> ...
>> -         net = skb->dev->nd_net;
>> +         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
>>From my POW the we can just get skb->dst->dev. Could we?
> 
> I think that on IP level dealing with fragments we always have the
> destination entry. I'll recheck this tomorrow.


Unfortunately no, on the receive path connection tracking
will call ip_defrag before routing took place.

Re: 2.6.25-rc: Null dereference in ip_defrag

[Phil Oester]

On Mon, Mar 17, 2008 at 06:43:32PM +0100, Patrick McHardy wrote:
> Pavel Emelyanov wrote:
> > Can you check with this patch, please (untested, but should work)?
> 
> This is getting pretty ugly. Shouldn't
> 
> int ip_defrag(struct sk_buff *skb, u32 user)
> {
> ...
> -         net = skb->dev->nd_net;
> +         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
> 
> work as well?

I can try either patch, but Patrick's looks simpler and thus perhaps
a better candidate this late in the -rc stage.

Pavel - you concur?

Phil

Re: 2.6.25-rc: Null dereference in ip_defrag

[Pavel Emelyanov]

Phil Oester wrote:
> On Mon, Mar 17, 2008 at 06:43:32PM +0100, Patrick McHardy wrote:
>> Pavel Emelyanov wrote:
>>> Can you check with this patch, please (untested, but should work)?
>> This is getting pretty ugly. Shouldn't
>>
>> int ip_defrag(struct sk_buff *skb, u32 user)
>> {
>> ...
>> -         net = skb->dev->nd_net;
>> +         net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
>>
>> work as well?

It will, though I do not like such implicit decisions.

> I can try either patch, but Patrick's looks simpler and thus perhaps
> a better candidate this late in the -rc stage.
> 
> Pavel - you concur?

No one, except for a complaint, that we (me and Denis) warned other
net namespaces developers that getting a struct net from function
arguments, sockets, dst entries, devices, etc. is going to become a
pain in the ... anyway. We proposed to create current_net macro, that
would give us a correct net in any place... Such thing would allow us
to avoid this problem. But I'm afraid, that it's just too late to
change the model :(

> Phil
> 

Re: 2.6.25-rc: Null dereference in ip_defrag

[Pavel Emelyanov]

[snip]

>> I can try either patch, but Patrick's looks simpler and thus perhaps
>> a better candidate this late in the -rc stage.
>>
>> Pavel - you concur?
> 
> No one, except for a complaint, ...

I wonder what I was thinking about when I wrote this :(

Phil, I agree with Patrick's patch.

Re: 2.6.25-rc: Null dereference in ip_defrag

[Phil Oester]

On Tue, Mar 18, 2008 at 12:51:21PM +0300, Pavel Emelyanov wrote:
> >> I can try either patch, but Patrick's looks simpler and thus perhaps
> >> a better candidate this late in the -rc stage.
> >>
> >> Pavel - you concur?
> 
> Phil, I agree with Patrick's patch.

And it has survived 1.5 days without issue.  Patrick - will you be pushing
to David?

Phil

Re: 2.6.25-rc: Null dereference in ip_defrag

[Patrick McHardy]

Phil Oester wrote:
> On Tue, Mar 18, 2008 at 12:51:21PM +0300, Pavel Emelyanov wrote:
>>>> I can try either patch, but Patrick's looks simpler and thus perhaps
>>>> a better candidate this late in the -rc stage.
>>>>
>>>> Pavel - you concur?
>> Phil, I agree with Patrick's patch.
> 
> And it has survived 1.5 days without issue.  Patrick - will you be pushing
> to David?

Could you send the patch you've been using?

Re: 2.6.25-rc: Null dereference in ip_defrag

[Phil Oester]

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

On Thu, Mar 20, 2008 at 03:47:38PM +0100, Patrick McHardy wrote:
> >And it has survived 1.5 days without issue.  Patrick - will you be pushing
> >to David?
> 
> Could you send the patch you've been using?

Sure - attached below against current git.  I assume you will add your
signoff, but you can add

Tested-by: Phil Oester <kernel@linuxace.com>

Phil



[-- Attachment #2: patch-ipfrags --]
[-- Type: text/plain, Size: 480 bytes --]

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index a2e92f9..3b2e5ad 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -568,7 +568,7 @@ int ip_defrag(struct sk_buff *skb, u32 user)
 
 	IP_INC_STATS_BH(IPSTATS_MIB_REASMREQDS);
 
-	net = skb->dev->nd_net;
+	net = skb->dev ? skb->dev->nd_net : skb->dst->dev->nd_net;
 	/* Start by cleaning up the memory. */
 	if (atomic_read(&net->ipv4.frags.mem) > net->ipv4.frags.high_thresh)
 		ip_evictor(net);

Re: 2.6.25-rc: Null dereference in ip_defrag

[Patrick McHardy]

Phil Oester wrote:
> On Thu, Mar 20, 2008 at 03:47:38PM +0100, Patrick McHardy wrote:
>>> And it has survived 1.5 days without issue.  Patrick - will you be pushing
>>> to David?
>> Could you send the patch you've been using?
> 
> Sure - attached below against current git.  I assume you will add your
> signoff, but you can add
> 
> Tested-by: Phil Oester <kernel@linuxace.com>

Thanks,

Acked-by: Patrick McHardy <kaber@trash.net>