Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Alexey Dobriyan]

On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
> Scenario: no ipv6 default route set.

> # ip -f inet6 route get fec0::1
> 
> BUG: unable to handle kernel NULL pointer dereference at 00000000
> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
> EIP is at rt6_fill_node+0x175/0x3b0
>  ip6_route_output+0x50/0xa0
>  ip6_pol_route_output+0x0/0x20
>  inet6_rtm_getroute+0x16e/0x200
>  inet6_rtm_getroute+0x0/0x200
>  rtnetlink_rcv_msg+0x1b9/0x1f0
>  rtnetlink_rcv_msg+0x0/0x1f0
>  netlink_rcv_skb+0x8d/0xb0
>  rtnetlink_rcv+0x17/0x20
>  netlink_unicast+0x23d/0x270
>  memcpy_fromiovec+0x4a/0x70
>  netlink_sendmsg+0x1c1/0x290
>  sock_sendmsg+0xc5/0xf0
>  autoremove_wake_function+0x0/0x50
>  autoremove_wake_function+0x0/0x50
>  sock_sendmsg+0xc5/0xf0
>  copy_from_user+0x37/0x70
>  verify_iovec+0x2c/0x90
>  sys_sendmsg+0x10a/0x220

0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
2186                    } else
2187    #endif
2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
2189            } else if (dst) {
2190                    struct in6_addr saddr_buf;
2191      ====>         if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
					       ^^^^^^^^^^^^^^^^^^^^^^^^
					       		NULL

2192                                           dst, 0, &saddr_buf) == 0)
2193                            NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
2194            }

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Brian Haley]

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

Alexey Dobriyan wrote:
> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
>> Scenario: no ipv6 default route set.
> 
>> # ip -f inet6 route get fec0::1
>>
>> BUG: unable to handle kernel NULL pointer dereference at 00000000
>> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
>> EIP is at rt6_fill_node+0x175/0x3b0
> 
> 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
> 2186                    } else
> 2187    #endif
> 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
> 2189            } else if (dst) {
> 2190                    struct in6_addr saddr_buf;
> 2191      ====>         if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
> 					       ^^^^^^^^^^^^^^^^^^^^^^^^
> 					       		NULL
> 
> 2192                                           dst, 0, &saddr_buf) == 0)
> 2193                            NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
> 2194            }

The commit that changed this can't be reverted easily, but the patch 
below works for me.

Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input 
device present in the dst entry.

Signed-off-by: Brian Haley <brian.haley@hp.com>



[-- Attachment #2: idev.patch --]
[-- Type: text/x-patch, Size: 551 bytes --]

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5a3e87e..303a245 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2188,7 +2188,8 @@ static int rt6_fill_node(struct sk_buff *skb, struct rt6_info *rt,
 			NLA_PUT_U32(skb, RTA_IIF, iif);
 	} else if (dst) {
 		struct in6_addr saddr_buf;
-		if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
+		struct inet6_dev *idev = ip6_dst_idev(&rt->u.dst);
+		if (ipv6_dev_get_saddr(idev ? idev->dev : NULL,
 				       dst, 0, &saddr_buf) == 0)
 			NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
 	}

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[John Gumb]

looks good to me.

thanks people.

John
On Fri, 2008-08-08 at 00:57 -0400, Brian Haley wrote:
> Alexey Dobriyan wrote:
> > On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
> >> Scenario: no ipv6 default route set.
> > 
> >> # ip -f inet6 route get fec0::1
> >>
> >> BUG: unable to handle kernel NULL pointer dereference at 00000000
> >> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
> >> EIP is at rt6_fill_node+0x175/0x3b0
> > 
> > 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
> > 2186                    } else
> > 2187    #endif
> > 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
> > 2189            } else if (dst) {
> > 2190                    struct in6_addr saddr_buf;
> > 2191      ====>         if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
> > 					       ^^^^^^^^^^^^^^^^^^^^^^^^
> > 					       		NULL
> > 
> > 2192                                           dst, 0, &saddr_buf) == 0)
> > 2193                            NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
> > 2194            }
> 
> The commit that changed this can't be reverted easily, but the patch 
> below works for me.
> 
> Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input 
> device present in the dst entry.
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> 

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Eugene Teo]

Brian Haley wrote:
> Alexey Dobriyan wrote:
>> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
>>> Scenario: no ipv6 default route set.
>>
>>> # ip -f inet6 route get fec0::1
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at 00000000
>>> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
>>> EIP is at rt6_fill_node+0x175/0x3b0
>>
>> 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
>> 2186                    } else
>> 2187    #endif
>> 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
>> 2189            } else if (dst) {
>> 2190                    struct in6_addr saddr_buf;
>> 2191      ====>         if
>> (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
>>                            ^^^^^^^^^^^^^^^^^^^^^^^^
>>                                    NULL
>>
>> 2192                                           dst, 0, &saddr_buf) == 0)
>> 2193                            NLA_PUT(skb, RTA_PREFSRC, 16,
>> &saddr_buf);
>> 2194            }
> 
> The commit that changed this can't be reverted easily, but the patch
> below works for me.
> 
> Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input
> device present in the dst entry.
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>

Cc: Stable <stable@kernel.org>

Eugene

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[David Miller]

From: Eugene Teo <eteo@redhat.com>
Date: Mon, 11 Aug 2008 16:40:24 +0800

> Brian Haley wrote:
> > Alexey Dobriyan wrote:
> >> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
> >>> Scenario: no ipv6 default route set.
> >>
> >>> # ip -f inet6 route get fec0::1
> >>>
> >>> BUG: unable to handle kernel NULL pointer dereference at 00000000
> >>> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
> >>> EIP is at rt6_fill_node+0x175/0x3b0
> >>
> >> 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
> >> 2186                    } else
> >> 2187    #endif
> >> 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
> >> 2189            } else if (dst) {
> >> 2190                    struct in6_addr saddr_buf;
> >> 2191      ====>         if
> >> (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
> >>                            ^^^^^^^^^^^^^^^^^^^^^^^^
> >>                                    NULL
> >>
> >> 2192                                           dst, 0, &saddr_buf) == 0)
> >> 2193                            NLA_PUT(skb, RTA_PREFSRC, 16,
> >> &saddr_buf);
> >> 2194            }
> > 
> > The commit that changed this can't be reverted easily, but the patch
> > below works for me.
> > 
> > Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input
> > device present in the dst entry.
> > 
> > Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> Cc: Stable <stable@kernel.org>

We've already determined from one tester that the behavior is
changing with Brian's patch.  Furthermore I haven't applied it
to mainline so it isn't anywhere near being submittable for
-stable.

I submit all relevant networking bug fixes to -stable when they are
ready and in a proper state to be submitted, you don't have to do it
for me.  Sending me a gentle reminder or nudge, on the other hand, is
fine.

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Eugene Teo]

David Miller wrote:
> From: Eugene Teo <eteo@redhat.com>
> Date: Mon, 11 Aug 2008 16:40:24 +0800
> 
>> Brian Haley wrote:
>>> Alexey Dobriyan wrote:
>>>> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
>>>>> Scenario: no ipv6 default route set.
>>>>> # ip -f inet6 route get fec0::1
>>>>>
>>>>> BUG: unable to handle kernel NULL pointer dereference at 00000000
>>>>> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
>>>>> EIP is at rt6_fill_node+0x175/0x3b0
>>>> 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
>>>> 2186                    } else
>>>> 2187    #endif
>>>> 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
>>>> 2189            } else if (dst) {
>>>> 2190                    struct in6_addr saddr_buf;
>>>> 2191      ====>         if
>>>> (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
>>>>                            ^^^^^^^^^^^^^^^^^^^^^^^^
>>>>                                    NULL
>>>>
>>>> 2192                                           dst, 0, &saddr_buf) == 0)
>>>> 2193                            NLA_PUT(skb, RTA_PREFSRC, 16,
>>>> &saddr_buf);
>>>> 2194            }
>>> The commit that changed this can't be reverted easily, but the patch
>>> below works for me.
>>>
>>> Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input
>>> device present in the dst entry.
>>>
>>> Signed-off-by: Brian Haley <brian.haley@hp.com>
>> Cc: Stable <stable@kernel.org>
> 
> We've already determined from one tester that the behavior is
> changing with Brian's patch.  Furthermore I haven't applied it
> to mainline so it isn't anywhere near being submittable for
> -stable.

With the patch I posted, this is the behaviour I get:

$ ip -f inet6 route get fec0::1
unreachable fec0::1 dev lo  table unspec  proto none  src
fe80::214:4fff:fe0f:7332  metric -1  error -101 hoplimit 255

John emailed me that he will be testing this patch. I have not tested
Brian's patch.

> I submit all relevant networking bug fixes to -stable when they are
> ready and in a proper state to be submitted, you don't have to do it
> for me.  Sending me a gentle reminder or nudge, on the other hand, is
> fine.

Thanks for letting me know. I wasn't familiar, but I welcome the hint.

Thanks,
Eugene

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Eugene Teo]

Eugene Teo wrote:
> David Miller wrote:
>> From: Eugene Teo <eteo@redhat.com>
>> Date: Mon, 11 Aug 2008 16:40:24 +0800
[...]
> With the patch I posted, this is the behaviour I get:
> 
> $ ip -f inet6 route get fec0::1
> unreachable fec0::1 dev lo  table unspec  proto none  src
> fe80::214:4fff:fe0f:7332  metric -1  error -101 hoplimit 255

Ok, so there's a mistake in my patch. It should return the loopback MAC
address instead. I'm wondering if the fix should be related to
initialising rt6i_idev in addrconf_init routine like in the upstream
commit: c62dba9011b93fd88fde929848582b2a98309878. The code changed quite
a lot.

Eugene

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Eugene Teo]

Eugene Teo wrote:
> Eugene Teo wrote:
>> David Miller wrote:
>>> From: Eugene Teo <eteo@redhat.com>
>>> Date: Mon, 11 Aug 2008 16:40:24 +0800
> [...]
>> With the patch I posted, this is the behaviour I get:
>>
>> $ ip -f inet6 route get fec0::1
>> unreachable fec0::1 dev lo  table unspec  proto none  src
>> fe80::214:4fff:fe0f:7332  metric -1  error -101 hoplimit 255
> 
> Ok, so there's a mistake in my patch. It should return the loopback MAC

s/MAC/ipv6/

> address instead. I'm wondering if the fix should be related to
> initialising rt6i_idev in addrconf_init routine like in the upstream
> commit: c62dba9011b93fd88fde929848582b2a98309878. The code changed quite
> a lot.

So I checked, it's initialised in ip6_route_init().

Eugene

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Eugene Teo]

Hi,

<quote sender="Brian Haley">
> Alexey Dobriyan wrote:
>> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
[...]
> The commit that changed this can't be reverted easily, but the patch  
> below works for me.
>
> Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input  
> device present in the dst entry.
>
> Signed-off-by: Brian Haley <brian.haley@hp.com>

I think it's better to use a helper routine like ipv6_get_saddr to make
sure that both dst and rt6i_idev arguments are checked for NULL.

I have compiled, and tested the patch.

Thanks,
Eugene

---
Fix NULL pointer dereference in rt6_fill_node().

# ip -f inet6 route get fec0::1

BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
EIP is at rt6_fill_node+0x175/0x3b0

Cc: Stable <stable@kernel.org>
Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 include/net/addrconf.h  |    4 ++++
 include/net/ip6_fib.h   |    2 +-
 net/ipv6/addrconf.c     |    8 ++++++++
 net/ipv6/fib6_rules.c   |    3 +--
 net/ipv6/ip6_output.c   |    3 +--
 net/ipv6/route.c        |    3 +--
 net/ipv6/xfrm6_policy.c |    3 +--
 net/sctp/ipv6.c         |    3 +--
 8 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 06b2814..c43ad8e 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -80,6 +80,10 @@ extern struct inet6_ifaddr      *ipv6_get_ifaddr(struct net *net,
 						 struct net_device *dev,
 						 int strict);
 
+extern int			ipv6_get_saddr(struct dst_entry *dst,
+					       const struct in6_addr *daddr,
+					       unsigned int srcprefs,
+					       struct in6_addr *saddr);
 extern int			ipv6_dev_get_saddr(struct net_device *dev, 
 					       const struct in6_addr *daddr,
 					       unsigned int srcprefs,
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 7c5c0f7..73f7f6b 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -118,7 +118,7 @@ struct rt6_info
 
 static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 {
-	return ((struct rt6_info *)dst)->rt6i_idev;
+	return dst ? ((struct rt6_info *)dst)->rt6i_idev : NULL;
 }
 
 struct fib6_walker_t
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a7842c5..55647bd 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1239,6 +1239,14 @@ try_nextdev:
 	return 0;
 }
 
+int ipv6_get_saddr(struct dst_entry *dst, const struct in6_addr *daddr,
+		    unsigned int srcprefs, struct in6_addr *saddr)
+{
+	struct inet6_dev *idev = ip6_dst_idev(dst);
+	return ipv6_dev_get_saddr(idev ? idev->dev : NULL, daddr, srcprefs, saddr);
+
+}
+EXPORT_SYMBOL(ipv6_get_saddr);
 EXPORT_SYMBOL(ipv6_dev_get_saddr);
 
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 8d05527..27c859c 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -93,8 +93,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 			if (flags & RT6_LOOKUP_F_SRCPREF_COA)
 				srcprefs |= IPV6_PREFER_SRC_COA;
 
-			if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-					       &flp->fl6_dst, srcprefs,
+			if (ipv6_get_saddr(&rt->u.dst, &flp->fl6_dst, srcprefs,
 					       &saddr))
 				goto again;
 			if (!ipv6_prefix_equal(&saddr, &r->src.addr,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a4402de..fc5f4f4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -934,8 +934,7 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 		goto out_err_release;
 
 	if (ipv6_addr_any(&fl->fl6_src)) {
-		err = ipv6_dev_get_saddr(ip6_dst_idev(*dst)->dev,
-					 &fl->fl6_dst,
+		err = ipv6_get_saddr(*dst, &fl->fl6_dst,
 					 sk ? inet6_sk(sk)->srcprefs : 0,
 					 &fl->fl6_src);
 		if (err)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5a3e87e..950d709 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2188,8 +2188,7 @@ static int rt6_fill_node(struct sk_buff *skb, struct rt6_info *rt,
 			NLA_PUT_U32(skb, RTA_IIF, iif);
 	} else if (dst) {
 		struct in6_addr saddr_buf;
-		if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-				       dst, 0, &saddr_buf) == 0)
+		if (ipv6_get_saddr(&rt->u.dst, dst, 0, &saddr_buf) == 0)
 			NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
 	}
 
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 8f1e054..ad582bd 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -57,8 +57,7 @@ static int xfrm6_get_saddr(xfrm_address_t *saddr, xfrm_address_t *daddr)
 	if (IS_ERR(dst))
 		return -EHOSTUNREACH;
 
-	ipv6_dev_get_saddr(ip6_dst_idev(dst)->dev,
-			   (struct in6_addr *)&daddr->a6, 0,
+	ipv6_get_saddr(dst, (struct in6_addr *)&daddr->a6, 0,
 			   (struct in6_addr *)&saddr->a6);
 	dst_release(dst);
 	return 0;
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 483a01d..0fa9411 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -319,8 +319,7 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
 			  __func__, asoc, dst, NIP6(daddr->v6.sin6_addr));
 
 	if (!asoc) {
-		ipv6_dev_get_saddr(dst ? ip6_dst_idev(dst)->dev : NULL,
-				   &daddr->v6.sin6_addr,
+		ipv6_get_saddr(dst, &daddr->v6.sin6_addr,
 				   inet6_sk(&sk->inet.sk)->srcprefs,
 				   &saddr->v6.sin6_addr);
 		SCTP_DEBUG_PRINTK("saddr from ipv6_get_saddr: " NIP6_FMT "\n",

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175

[Brian Haley]

Eugene Teo wrote:
> I think it's better to use a helper routine like ipv6_get_saddr to make
> sure that both dst and rt6i_idev arguments are checked for NULL.
> 
> I have compiled, and tested the patch.
> 
> Thanks,
> Eugene
> 
> ---
> Fix NULL pointer dereference in rt6_fill_node().
> 
> # ip -f inet6 route get fec0::1
> 
> BUG: unable to handle kernel NULL pointer dereference at 00000000
> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
> EIP is at rt6_fill_node+0x175/0x3b0
> 
> Cc: Stable <stable@kernel.org>
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>

Acked-by: Brian Haley <brian.haley@hp.com>

But Yoshfuji might have another opinion since he did the work to remove 
ipv6_get_saddr() in the first place.

-Brian

RE: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26,ip6_route_output, rt6_fill_node+0x175

[John Gumb]

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

Folks

I've enclosed patch from Eugene just so we all know which patch we're
talking about. It 'works' according to the following definition:

a) Fixed OOPS
b) runs overnight in our test network. This run doesn't do much specific
ipv6 testing - but clearly what's there is catching stuff :-;

cheers

John
-----Original Message-----
From: Eugene Teo [mailto:eugeneteo@kernel.sg] 
Sent: 11 August 2008 12:04
To: Brian Haley
Cc: Alexey Dobriyan; John Gumb; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org; YOSHIFUJI Hideaki
Subject: Re: OOPS, ip -f inet6 route get fec0::1,
linux-2.6.26,ip6_route_output, rt6_fill_node+0x175

Hi,

<quote sender="Brian Haley">
> Alexey Dobriyan wrote:
>> On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
[...]
> The commit that changed this can't be reverted easily, but the patch  
> below works for me.
>
> Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input  
> device present in the dst entry.
>
> Signed-off-by: Brian Haley <brian.haley@hp.com>

I think it's better to use a helper routine like ipv6_get_saddr to make
sure that both dst and rt6i_idev arguments are checked for NULL.

I have compiled, and tested the patch.

Thanks,
Eugene

---
Fix NULL pointer dereference in rt6_fill_node().

# ip -f inet6 route get fec0::1

BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
EIP is at rt6_fill_node+0x175/0x3b0

Cc: Stable <stable@kernel.org>
Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 include/net/addrconf.h  |    4 ++++
 include/net/ip6_fib.h   |    2 +-
 net/ipv6/addrconf.c     |    8 ++++++++
 net/ipv6/fib6_rules.c   |    3 +--
 net/ipv6/ip6_output.c   |    3 +--
 net/ipv6/route.c        |    3 +--
 net/ipv6/xfrm6_policy.c |    3 +--
 net/sctp/ipv6.c         |    3 +--
 8 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 06b2814..c43ad8e 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -80,6 +80,10 @@ extern struct inet6_ifaddr
*ipv6_get_ifaddr(struct net *net,
 						 struct net_device *dev,
 						 int strict);
 
+extern int			ipv6_get_saddr(struct dst_entry *dst,
+					       const struct in6_addr
*daddr,
+					       unsigned int srcprefs,
+					       struct in6_addr *saddr);
 extern int			ipv6_dev_get_saddr(struct net_device
*dev, 
 					       const struct in6_addr
*daddr,
 					       unsigned int srcprefs,
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 7c5c0f7..73f7f6b 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -118,7 +118,7 @@ struct rt6_info
 
 static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 {
-	return ((struct rt6_info *)dst)->rt6i_idev;
+	return dst ? ((struct rt6_info *)dst)->rt6i_idev : NULL;
 }
 
 struct fib6_walker_t
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a7842c5..55647bd 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1239,6 +1239,14 @@ try_nextdev:
 	return 0;
 }
 
+int ipv6_get_saddr(struct dst_entry *dst, const struct in6_addr *daddr,
+		    unsigned int srcprefs, struct in6_addr *saddr)
+{
+	struct inet6_dev *idev = ip6_dst_idev(dst);
+	return ipv6_dev_get_saddr(idev ? idev->dev : NULL, daddr,
srcprefs, saddr);
+
+}
+EXPORT_SYMBOL(ipv6_get_saddr);
 EXPORT_SYMBOL(ipv6_dev_get_saddr);
 
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 8d05527..27c859c 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -93,8 +93,7 @@ static int fib6_rule_action(struct fib_rule *rule,
struct flowi *flp,
 			if (flags & RT6_LOOKUP_F_SRCPREF_COA)
 				srcprefs |= IPV6_PREFER_SRC_COA;
 
-			if
(ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-					       &flp->fl6_dst, srcprefs,
+			if (ipv6_get_saddr(&rt->u.dst, &flp->fl6_dst,
srcprefs,
 					       &saddr))
 				goto again;
 			if (!ipv6_prefix_equal(&saddr, &r->src.addr,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a4402de..fc5f4f4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -934,8 +934,7 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 		goto out_err_release;
 
 	if (ipv6_addr_any(&fl->fl6_src)) {
-		err = ipv6_dev_get_saddr(ip6_dst_idev(*dst)->dev,
-					 &fl->fl6_dst,
+		err = ipv6_get_saddr(*dst, &fl->fl6_dst,
 					 sk ? inet6_sk(sk)->srcprefs :
0,
 					 &fl->fl6_src);
 		if (err)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5a3e87e..950d709 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2188,8 +2188,7 @@ static int rt6_fill_node(struct sk_buff *skb,
struct rt6_info *rt,
 			NLA_PUT_U32(skb, RTA_IIF, iif);
 	} else if (dst) {
 		struct in6_addr saddr_buf;
-		if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-				       dst, 0, &saddr_buf) == 0)
+		if (ipv6_get_saddr(&rt->u.dst, dst, 0, &saddr_buf) == 0)
 			NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
 	}
 
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 8f1e054..ad582bd 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -57,8 +57,7 @@ static int xfrm6_get_saddr(xfrm_address_t *saddr,
xfrm_address_t *daddr)
 	if (IS_ERR(dst))
 		return -EHOSTUNREACH;
 
-	ipv6_dev_get_saddr(ip6_dst_idev(dst)->dev,
-			   (struct in6_addr *)&daddr->a6, 0,
+	ipv6_get_saddr(dst, (struct in6_addr *)&daddr->a6, 0,
 			   (struct in6_addr *)&saddr->a6);
 	dst_release(dst);
 	return 0;
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 483a01d..0fa9411 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -319,8 +319,7 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
 			  __func__, asoc, dst,
NIP6(daddr->v6.sin6_addr));
 
 	if (!asoc) {
-		ipv6_dev_get_saddr(dst ? ip6_dst_idev(dst)->dev : NULL,
-				   &daddr->v6.sin6_addr,
+		ipv6_get_saddr(dst, &daddr->v6.sin6_addr,
 				   inet6_sk(&sk->inet.sk)->srcprefs,
 				   &saddr->v6.sin6_addr);
 		SCTP_DEBUG_PRINTK("saddr from ipv6_get_saddr: " NIP6_FMT
"\n",

[-- Attachment #2: bug#52312-idev.patch --]
[-- Type: application/octet-stream, Size: 4804 bytes --]

Fix NULL pointer dereference in rt6_fill_node().

# ip -f inet6 route get fec0::1

BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
EIP is at rt6_fill_node+0x175/0x3b0

Cc: Stable <stable@kernel.org>
Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 include/net/addrconf.h  |    4 ++++
 include/net/ip6_fib.h   |    2 +-
 net/ipv6/addrconf.c     |    8 ++++++++
 net/ipv6/fib6_rules.c   |    3 +--
 net/ipv6/ip6_output.c   |    3 +--
 net/ipv6/route.c        |    3 +--
 net/ipv6/xfrm6_policy.c |    3 +--
 net/sctp/ipv6.c         |    3 +--
 8 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 06b2814..c43ad8e 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -80,6 +80,10 @@ extern struct inet6_ifaddr      *ipv6_get_ifaddr(struct net *net,
 						 struct net_device *dev,
 						 int strict);
 
+extern int			ipv6_get_saddr(struct dst_entry *dst,
+					       const struct in6_addr *daddr,
+					       unsigned int srcprefs,
+					       struct in6_addr *saddr);
 extern int			ipv6_dev_get_saddr(struct net_device *dev, 
 					       const struct in6_addr *daddr,
 					       unsigned int srcprefs,
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 7c5c0f7..73f7f6b 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -118,7 +118,7 @@ struct rt6_info
 
 static inline struct inet6_dev *ip6_dst_idev(struct dst_entry *dst)
 {
-	return ((struct rt6_info *)dst)->rt6i_idev;
+	return dst ? ((struct rt6_info *)dst)->rt6i_idev : NULL;
 }
 
 struct fib6_walker_t
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a7842c5..55647bd 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1239,6 +1239,14 @@ try_nextdev:
 	return 0;
 }
 
+int ipv6_get_saddr(struct dst_entry *dst, const struct in6_addr *daddr,
+		    unsigned int srcprefs, struct in6_addr *saddr)
+{
+	struct inet6_dev *idev = ip6_dst_idev(dst);
+	return ipv6_dev_get_saddr(idev ? idev->dev : NULL, daddr, srcprefs, saddr);
+
+}
+EXPORT_SYMBOL(ipv6_get_saddr);
 EXPORT_SYMBOL(ipv6_dev_get_saddr);
 
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 8d05527..27c859c 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -93,8 +93,7 @@ static int fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 			if (flags & RT6_LOOKUP_F_SRCPREF_COA)
 				srcprefs |= IPV6_PREFER_SRC_COA;
 
-			if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-					       &flp->fl6_dst, srcprefs,
+			if (ipv6_get_saddr(&rt->u.dst, &flp->fl6_dst, srcprefs,
 					       &saddr))
 				goto again;
 			if (!ipv6_prefix_equal(&saddr, &r->src.addr,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a4402de..fc5f4f4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -934,8 +934,7 @@ static int ip6_dst_lookup_tail(struct sock *sk,
 		goto out_err_release;
 
 	if (ipv6_addr_any(&fl->fl6_src)) {
-		err = ipv6_dev_get_saddr(ip6_dst_idev(*dst)->dev,
-					 &fl->fl6_dst,
+		err = ipv6_get_saddr(*dst, &fl->fl6_dst,
 					 sk ? inet6_sk(sk)->srcprefs : 0,
 					 &fl->fl6_src);
 		if (err)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5a3e87e..950d709 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2188,8 +2188,7 @@ static int rt6_fill_node(struct sk_buff *skb, struct rt6_info *rt,
 			NLA_PUT_U32(skb, RTA_IIF, iif);
 	} else if (dst) {
 		struct in6_addr saddr_buf;
-		if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
-				       dst, 0, &saddr_buf) == 0)
+		if (ipv6_get_saddr(&rt->u.dst, dst, 0, &saddr_buf) == 0)
 			NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
 	}
 
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 8f1e054..ad582bd 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -57,8 +57,7 @@ static int xfrm6_get_saddr(xfrm_address_t *saddr, xfrm_address_t *daddr)
 	if (IS_ERR(dst))
 		return -EHOSTUNREACH;
 
-	ipv6_dev_get_saddr(ip6_dst_idev(dst)->dev,
-			   (struct in6_addr *)&daddr->a6, 0,
+	ipv6_get_saddr(dst, (struct in6_addr *)&daddr->a6, 0,
 			   (struct in6_addr *)&saddr->a6);
 	dst_release(dst);
 	return 0;
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 483a01d..0fa9411 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -319,8 +319,7 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
 			  __func__, asoc, dst, NIP6(daddr->v6.sin6_addr));
 
 	if (!asoc) {
-		ipv6_dev_get_saddr(dst ? ip6_dst_idev(dst)->dev : NULL,
-				   &daddr->v6.sin6_addr,
+		ipv6_get_saddr(dst, &daddr->v6.sin6_addr,
 				   inet6_sk(&sk->inet.sk)->srcprefs,
 				   &saddr->v6.sin6_addr);
 		SCTP_DEBUG_PRINTK("saddr from ipv6_get_saddr: " NIP6_FMT "\n",

Re: OOPS, ip -f inet6 route get fec0::1, linux-2.6.26,ip6_route_output, rt6_fill_node+0x175

[David Miller]

From: "John Gumb" <john.gumb@tandberg.com>
Date: Tue, 12 Aug 2008 11:11:47 +0200

> I've enclosed patch from Eugene just so we all know which patch we're
> talking about. It 'works' according to the following definition:
> 
> a) Fixed OOPS
> b) runs overnight in our test network. This run doesn't do much specific
> ipv6 testing - but clearly what's there is catching stuff :-;

While Eugene's patch seems mostly fine, it's a bit over the top
to cure this OOPS and get backported to -stable I think.

So I've applied Brian's patch, as below, because we have many
other reports that it fixes the crash too.

I'd appreciate it if you'd test Brian's patch as well as you
tested Eugene's as this is what will go into the tree for the
time being.

We can reinvestigate Eugene's patch, but one thing I don't like
about it is that it adds this silly NULL check when that is
totally unnecessary in the vast majority of these call sites.

Yes yes, I'll submit this to stable too before someone bugs me
about that again.  I'll first let this sit and get tested for
a few days before I do that submission so don't panic if you
don't see it for a few days.

commit 5e0115e500fe9dd2ca11e6f92db9123204f1327a
Author: Brian Haley <brian.haley@hp.com>
Date:   Wed Aug 13 01:58:57 2008 -0700

    ipv6: Fix OOPS, ip -f inet6 route get fec0::1, linux-2.6.26, ip6_route_output, rt6_fill_node+0x175
    
    Alexey Dobriyan wrote:
    > On Thu, Aug 07, 2008 at 07:00:56PM +0200, John Gumb wrote:
    >> Scenario: no ipv6 default route set.
    >
    >> # ip -f inet6 route get fec0::1
    >>
    >> BUG: unable to handle kernel NULL pointer dereference at 00000000
    >> IP: [<c0369b85>] rt6_fill_node+0x175/0x3b0
    >> EIP is at rt6_fill_node+0x175/0x3b0
    >
    > 0xffffffff80424dd3 is in rt6_fill_node (net/ipv6/route.c:2191).
    > 2186                    } else
    > 2187    #endif
    > 2188                            NLA_PUT_U32(skb, RTA_IIF, iif);
    > 2189            } else if (dst) {
    > 2190                    struct in6_addr saddr_buf;
    > 2191      ====>         if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
    >					       ^^^^^^^^^^^^^^^^^^^^^^^^
    >											NULL
    >
    > 2192                                           dst, 0, &saddr_buf) == 0)
    > 2193                            NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
    > 2194            }
    
    The commit that changed this can't be reverted easily, but the patch
    below works for me.
    
    Fix NULL de-reference in rt6_fill_node() when there's no IPv6 input
    device present in the dst entry.
    
    Signed-off-by: Brian Haley <brian.haley@hp.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5a3e87e..41b165f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2187,8 +2187,9 @@ static int rt6_fill_node(struct sk_buff *skb, struct rt6_info *rt,
 #endif
 			NLA_PUT_U32(skb, RTA_IIF, iif);
 	} else if (dst) {
+		struct inet6_dev *idev = ip6_dst_idev(&rt->u.dst);
 		struct in6_addr saddr_buf;
-		if (ipv6_dev_get_saddr(ip6_dst_idev(&rt->u.dst)->dev,
+		if (ipv6_dev_get_saddr(idev ? idev->dev : NULL,
 				       dst, 0, &saddr_buf) == 0)
 			NLA_PUT(skb, RTA_PREFSRC, 16, &saddr_buf);
 	}