* [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
@ 2012-06-15 9:00 Thomas Graf
2012-06-15 10:56 ` Neil Horman
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Graf @ 2012-06-15 9:00 UTC (permalink / raw)
To: davem; +Cc: netdev
/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.
This opens up a short time frame to access fib_table_hash with its pants
down.
fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
include/net/ip6_fib.h | 2 ++
net/ipv6/ip6_fib.c | 18 +++++++++++-------
net/ipv6/route.c | 16 +++++++++++-----
3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 0ae759a..209af13 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -271,6 +271,8 @@ extern void fib6_run_gc(unsigned long expires,
extern void fib6_gc_cleanup(void);
extern int fib6_init(void);
+extern int fib6_init_late(void);
+extern void fib6_cleanup_late(void);
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
extern int fib6_rules_init(void);
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 74c21b9..fbd4aff 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1692,21 +1692,25 @@ int __init fib6_init(void)
ret = register_pernet_subsys(&fib6_net_ops);
if (ret)
goto out_kmem_cache_create;
-
- ret = __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
- NULL);
- if (ret)
- goto out_unregister_subsys;
out:
return ret;
-out_unregister_subsys:
- unregister_pernet_subsys(&fib6_net_ops);
out_kmem_cache_create:
kmem_cache_destroy(fib6_node_kmem);
goto out;
}
+int __init fib6_init_late(void)
+{
+ return __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
+ NULL);
+}
+
+void fib6_cleanup_late(void)
+{
+ rtnl_unregister(PF_INET6, RTM_GETROUTE);
+}
+
void fib6_gc_cleanup(void)
{
unregister_pernet_subsys(&fib6_net_ops);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 999a982..dc60bf5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3018,10 +3018,14 @@ int __init ip6_route_init(void)
if (ret)
goto out_kmem_cache;
- ret = register_pernet_subsys(&ip6_route_net_ops);
+ ret = fib6_init();
if (ret)
goto out_dst_entries;
+ ret = register_pernet_subsys(&ip6_route_net_ops);
+ if (ret)
+ goto out_fib6_init;
+
ip6_dst_blackhole_ops.kmem_cachep = ip6_dst_ops_template.kmem_cachep;
/* Registering of the loopback is done before this portion of code,
@@ -3035,13 +3039,13 @@ int __init ip6_route_init(void)
init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev;
init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev);
#endif
- ret = fib6_init();
+ ret = fib6_init_late();
if (ret)
goto out_register_subsys;
ret = xfrm6_init();
if (ret)
- goto out_fib6_init;
+ goto out_fib6_init_late;
ret = fib6_rules_init();
if (ret)
@@ -3064,10 +3068,12 @@ fib6_rules_init:
fib6_rules_cleanup();
xfrm6_init:
xfrm6_fini();
-out_fib6_init:
- fib6_gc_cleanup();
+out_fib6_init_late:
+ fib6_cleanup_late();
out_register_subsys:
unregister_pernet_subsys(&ip6_route_net_ops);
+out_fib6_init:
+ fib6_gc_cleanup();
out_dst_entries:
dst_entries_destroy(&ip6_dst_blackhole_ops);
out_kmem_cache:
--
1.7.7.6
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-15 9:00 [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route Thomas Graf
@ 2012-06-15 10:56 ` Neil Horman
2012-06-15 22:32 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2012-06-15 10:56 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, netdev
On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:
> /proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
> handler is installed in ip6_route_net_init() whereas fib_table_hash is
> allocated in fib6_net_init() _after_ the proc handler has been installed.
>
> This opens up a short time frame to access fib_table_hash with its pants
> down.
>
> fib6_init() as a whole can't be moved to an earlier position as it also
> registers the rtnetlink message handlers which should be registered at
> the end. Therefore split it into fib6_init() which is run early and
> fib6_init_late() to register the rtnetlink message handlers.
>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-15 10:56 ` Neil Horman
@ 2012-06-15 22:32 ` David Miller
2012-06-16 5:15 ` David Miller
2012-06-19 11:36 ` Thomas Graf
0 siblings, 2 replies; 11+ messages in thread
From: David Miller @ 2012-06-15 22:32 UTC (permalink / raw)
To: nhorman; +Cc: tgraf, netdev
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 15 Jun 2012 06:56:55 -0400
> On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:
>> /proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
>> handler is installed in ip6_route_net_init() whereas fib_table_hash is
>> allocated in fib6_net_init() _after_ the proc handler has been installed.
>>
>> This opens up a short time frame to access fib_table_hash with its pants
>> down.
>>
>> fib6_init() as a whole can't be moved to an earlier position as it also
>> registers the rtnetlink message handlers which should be registered at
>> the end. Therefore split it into fib6_init() which is run early and
>> fib6_init_late() to register the rtnetlink message handlers.
>>
>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
Applied.
Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-15 22:32 ` David Miller
@ 2012-06-16 5:15 ` David Miller
2012-06-16 8:13 ` David Miller
` (2 more replies)
2012-06-19 11:36 ` Thomas Graf
1 sibling, 3 replies; 11+ messages in thread
From: David Miller @ 2012-06-16 5:15 UTC (permalink / raw)
To: nhorman; +Cc: tgraf, netdev
From: David Miller <davem@davemloft.net>
Date: Fri, 15 Jun 2012 15:32:40 -0700 (PDT)
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Fri, 15 Jun 2012 06:56:55 -0400
>
>> On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:
>>> /proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
>>> handler is installed in ip6_route_net_init() whereas fib_table_hash is
>>> allocated in fib6_net_init() _after_ the proc handler has been installed.
>>>
>>> This opens up a short time frame to access fib_table_hash with its pants
>>> down.
>>>
>>> fib6_init() as a whole can't be moved to an earlier position as it also
>>> registers the rtnetlink message handlers which should be registered at
>>> the end. Therefore split it into fib6_init() which is run early and
>>> fib6_init_late() to register the rtnetlink message handlers.
>>>
>>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
>> Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
>
> Applied.
>
> Since you're snooping around in here, you might notice that on network
> namespace shutdown, we leak all user configured ipv6 FIB rules.
Thomas, this patch is buggy.
We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.
This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
initialized.
Any route lookup will crash when we dereference a root's ->leaf
because it will be NULL.
Please test your changes more thoroughly.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-16 5:15 ` David Miller
@ 2012-06-16 8:13 ` David Miller
2012-06-16 13:07 ` Neil Horman
2012-06-17 6:11 ` Thomas Graf
2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2012-06-16 8:13 UTC (permalink / raw)
To: nhorman; +Cc: tgraf, netdev
From: David Miller <davem@davemloft.net>
Date: Fri, 15 Jun 2012 22:15:02 -0700 (PDT)
> We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.
>
> This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
> initialized.
>
> Any route lookup will crash when we dereference a root's ->leaf
> because it will be NULL.
I've decided to revert this change for now.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-16 5:15 ` David Miller
2012-06-16 8:13 ` David Miller
@ 2012-06-16 13:07 ` Neil Horman
2012-06-16 22:22 ` David Miller
2012-06-17 6:11 ` Thomas Graf
2 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2012-06-16 13:07 UTC (permalink / raw)
To: David Miller; +Cc: tgraf, netdev
On Fri, Jun 15, 2012 at 10:15:02PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Fri, 15 Jun 2012 15:32:40 -0700 (PDT)
>
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Date: Fri, 15 Jun 2012 06:56:55 -0400
> >
> >> On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:
> >>> /proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
> >>> handler is installed in ip6_route_net_init() whereas fib_table_hash is
> >>> allocated in fib6_net_init() _after_ the proc handler has been installed.
> >>>
> >>> This opens up a short time frame to access fib_table_hash with its pants
> >>> down.
> >>>
> >>> fib6_init() as a whole can't be moved to an earlier position as it also
> >>> registers the rtnetlink message handlers which should be registered at
> >>> the end. Therefore split it into fib6_init() which is run early and
> >>> fib6_init_late() to register the rtnetlink message handlers.
> >>>
> >>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> >> Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
> >
> > Applied.
> >
> > Since you're snooping around in here, you might notice that on network
> > namespace shutdown, we leak all user configured ipv6 FIB rules.
>
> Thomas, this patch is buggy.
>
> We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.
>
> This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
> initialized.
>
> Any route lookup will crash when we dereference a root's ->leaf
> because it will be NULL.
>
Perhaps the flag on the proc file like we discussed might be the better way to
go after all here Thomas.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-16 13:07 ` Neil Horman
@ 2012-06-16 22:22 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2012-06-16 22:22 UTC (permalink / raw)
To: nhorman; +Cc: tgraf, netdev
From: Neil Horman <nhorman@tuxdriver.com>
Date: Sat, 16 Jun 2012 09:07:23 -0400
> Perhaps the flag on the proc file like we discussed might be the
> better way to go after all here Thomas.
I suspect Thomas's general approach was fine, he just was trying
to hard. :-)
Just seperate exactly the procfs, sysctl, etc. stuff into a completely
seperate set of network namespace ops, and do it dead last in the
initialization list.
At least that is how I would approach the fix.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-16 5:15 ` David Miller
2012-06-16 8:13 ` David Miller
2012-06-16 13:07 ` Neil Horman
@ 2012-06-17 6:11 ` Thomas Graf
2 siblings, 0 replies; 11+ messages in thread
From: Thomas Graf @ 2012-06-17 6:11 UTC (permalink / raw)
To: David Miller; +Cc: nhorman, netdev
On Fri, Jun 15, 2012 at 10:15:02PM -0700, David Miller wrote:
> Thomas, this patch is buggy.
>
> We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.
>
> This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
> initialized.
>
> Any route lookup will crash when we dereference a root's ->leaf
> because it will be NULL.
>
> Please test your changes more thoroughly.
Sorry, that this has slipped through. I must have booted the wrong
kernel. I even had this run by an external tester to confirm that
the original panic disapppeared...
I'll come up with a new fix.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-15 22:32 ` David Miller
2012-06-16 5:15 ` David Miller
@ 2012-06-19 11:36 ` Thomas Graf
2012-06-19 21:13 ` David Miller
1 sibling, 1 reply; 11+ messages in thread
From: Thomas Graf @ 2012-06-19 11:36 UTC (permalink / raw)
To: David Miller; +Cc: nhorman, netdev
On Fri, Jun 15, 2012 at 03:32:40PM -0700, David Miller wrote:
> Since you're snooping around in here, you might notice that on network
> namespace shutdown, we leak all user configured ipv6 FIB rules.
I looked into this. fib_rules_unregister() will free all rules
belonging to the address family in that namespace.
Or were you referring to other rules?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
2012-06-19 11:36 ` Thomas Graf
@ 2012-06-19 21:13 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2012-06-19 21:13 UTC (permalink / raw)
To: tgraf; +Cc: nhorman, netdev
From: Thomas Graf <tgraf@suug.ch>
Date: Tue, 19 Jun 2012 07:36:02 -0400
> On Fri, Jun 15, 2012 at 03:32:40PM -0700, David Miller wrote:
>> Since you're snooping around in here, you might notice that on network
>> namespace shutdown, we leak all user configured ipv6 FIB rules.
>
> I looked into this. fib_rules_unregister() will free all rules
> belonging to the address family in that namespace.
>
> Or were you referring to other rules?
Sorry, the leak I saw was for the fib6 tables, not the rules
themselves.
IPV4 has ip_fib_net_exit() which walks the FIB4 table hash
and releases everything.
I couldn't find the IPV6 counterpart. All I could find was code which
explicitly liberates the ipv6 main and local tables.
There is no ipv6 code I can find which traverses fib_table_hash and
liberates the dynamically generated tables.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route
@ 2012-06-16 9:46 Sedat Dilek
0 siblings, 0 replies; 11+ messages in thread
From: Sedat Dilek @ 2012-06-16 9:46 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Thomas Graf
Hi,
I pulled net.git#master on top of latest Linus upstream GIT.
The revert [1] fixes machine's kernel-panic.
Thanks.
Regards,
- Sedat -
[1] http://git.kernel.org/?p=linux/kernel/git/davem/net.git;a=commitdiff;h=e8803b6c387129059e04d9e14d49efda250a7361
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-06-19 21:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-15 9:00 [PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route Thomas Graf
2012-06-15 10:56 ` Neil Horman
2012-06-15 22:32 ` David Miller
2012-06-16 5:15 ` David Miller
2012-06-16 8:13 ` David Miller
2012-06-16 13:07 ` Neil Horman
2012-06-16 22:22 ` David Miller
2012-06-17 6:11 ` Thomas Graf
2012-06-19 11:36 ` Thomas Graf
2012-06-19 21:13 ` David Miller
2012-06-16 9:46 Sedat Dilek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.