Hi guys,
i hit some warnings when testing this patch on my local 4.4 kernel(arm64
chromebook) with KASAN & SLUB_DEBUG:
[ 9.919374] BUG: KASAN: use-after-free in
ip6_route_dev_notify+0x194/0x2bc at addr ffffffc0c9d4e480
[ 9.928469] Read of size 4 by task kworker/u12:3/124
[ 9.933463]
=============================================================================
[ 9.941686] BUG kmalloc-1024 (Not tainted): kasan: bad access detected
...
[ 10.741337] Workqueue: netns cleanup_net
[ 10.745300] Call trace:
[ 10.747776] [<ffffffc00020b30c>] dump_backtrace+0x0/0x200
[ 10.753203] [<ffffffc00020b530>] show_stack+0x24/0x30
[ 10.758284] [<ffffffc000602948>] dump_stack+0xa8/0xcc
[ 10.763364] [<ffffffc0003d8e90>] print_trailer+0x158/0x168
[ 10.768877] [<ffffffc0003d927c>] object_err+0x4c/0x5c
[ 10.773956] [<ffffffc0003dfef0>] kasan_report+0x338/0x4ec
[ 10.779383] [<ffffffc0003df05c>] __asan_load4+0x7c/0x84
[ 10.784640] [<ffffffc000c65d60>] ip6_route_dev_notify+0x194/0x2bc
[ 10.790763] [<ffffffc000258550>] notifier_call_chain+0x78/0xc0
[ 10.796625] [<ffffffc0002586f4>] raw_notifier_call_chain+0x3c/0x4c
[ 10.802835] [<ffffffc000b06118>] call_netdevice_notifiers_info+0x8c/0x9c
[ 10.809564] [<ffffffc000b061c4>] call_netdevice_notifiers+0x9c/0xcc
[ 10.815859] [<ffffffc000b146c8>] netdev_run_todo+0x224/0x3f0
[ 10.821547] [<ffffffc000b25054>] rtnl_unlock+0x14/0x1c
[ 10.826716] [<ffffffc000b0f6e0>] default_device_exit_batch+0x258/0x2a0
[ 10.833269] [<ffffffc000afe060>] ops_exit_list+0x74/0xdc
[ 10.838608] [<ffffffc000affd00>] cleanup_net+0x290/0x400
and also this:
[ 11.607268] BUG kmalloc-1024 (Tainted: G B ): Poison
overwritten
[ 11.607270]
-----------------------------------------------------------------------------
[ 11.607274] INFO: 0xffffffc0c9d4e478-0xffffffc0c9d4e478. First byte
0x67 instead of 0x6b
...
[ 11.607679] [<ffffffc0003d8e90>] print_trailer+0x158/0x168
[ 11.607683] [<ffffffc0003d8f78>] check_bytes_and_report+0xd8/0x13c
[ 11.607688] [<ffffffc0003d93c0>] check_object+0x134/0x230
[ 11.607692] [<ffffffc0003da7ac>] alloc_debug_processing+0x104/0x178
[ 11.607697] [<ffffffc0003dab0c>] ___slab_alloc.constprop.26+0x2ec/0x434
[ 11.607702] [<ffffffc0003dac9c>]
__slab_alloc.isra.23.constprop.25+0x48/0x5c
[ 11.607707] [<ffffffc0003deabc>] __kmalloc_track_caller+0x12c/0x338
it looks like the "struct inet6_dev" been touched after freed, and
refcnt changed(0xffffffc0c9d4e478, 376 bytes of struct inet6_dev) when
reusing this memory.
i think the problem would be we are assuming NETDEV_REGISTER and
NETDEV_UNREGISTER be paired in ip6_route_dev_notify:
> + if (event == NETDEV_REGISTER) {
> net->ipv6.ip6_null_entry->dst.dev = dev;
> net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> @@ -3718,6 +3721,12 @@ static int ip6_route_dev_notify(struct
notifier_block *this,
> net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
> net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
> #endif
> + } else if (event == NETDEV_UNREGISTER) {
> + in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev);
> +#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> + in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev);
> + in6_dev_put(net->ipv6.ip6_blk_hole_entry->rt6i_idev);
> +#endif
> }
but actually they are not guaranteed to be paired:
the netdev_run_todo(see the first dump stack above) would call
netdev_wait_allrefs to rebroadcast unregister notification multiple
times, unless timed out or all of the "struct net_device"'s refs released:
* This is called when unregistering network devices.
*
* Any protocol or device that holds a reference should register
* for netdevice notification, and cleanup and put back the
* reference if they receive an UNREGISTER event.
* We can get stuck here if buggy protocols don't correctly
* call dev_put.
*/
static void netdev_wait_allrefs(struct net_device *dev)
{
...
while (refcnt != 0) {
if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
rtnl_lock();
/* Rebroadcast unregister notification */
call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
__rtnl_unlock();
rcu_barrier();
rtnl_lock();
call_netdevice_notifiers(NETDEV_UNREGISTER_FINAL, dev);
On 05/05/2017 01:36 AM, WANG Cong wrote:
For each netns (except init_net), we initialize its null entry
in 3 places:
1) The template itself, as we use kmemdup()
2) Code around dst_init_metrics() in ip6_route_net_init()
3) ip6_route_dev_notify(), which is supposed to initialize it after
loopback registers
Unfortunately the last one still happens in a wrong order because
we expect to initialize net->ipv6.ip6_null_entry->rt6i_idev to
net->loopback_dev's idev, so we have to do that after we add
idev to it. However, this notifier has priority == 0 same as
ipv6_dev_notf, and ipv6_dev_notf is registered after
ip6_route_dev_notifier so it is called actually after
ip6_route_dev_notifier.
Fix it by picking a smaller priority for ip6_route_dev_notifier.
Also, we have to release the refcnt accordingly when unregistering
loopback_dev because device exit functions are called before subsys
exit functions.
Cc: David Ahern <dsah...@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
Acked-by: David Ahern <dsah...@gmail.com>
Tested-by: David Ahern <dsah...@gmail.com>
---
include/net/addrconf.h | 2 ++
net/ipv6/addrconf.c | 1 +
net/ipv6/route.c | 13 +++++++++++--
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 1aeb25d..6c0ee3c 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -20,6 +20,8 @@
#define ADDRCONF_TIMER_FUZZ (HZ / 4)
#define ADDRCONF_TIMER_FUZZ_MAX (HZ)
+#define ADDRCONF_NOTIFY_PRIORITY 0
+
#include <linux/in.h>
#include <linux/in6.h>
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 77a4bd5..8d297a7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3548,6 +3548,7 @@ static int addrconf_notify(struct notifier_block *this,
unsigned long event,
*/
static struct notifier_block ipv6_dev_notf = {
.notifier_call = addrconf_notify,
+ .priority = ADDRCONF_NOTIFY_PRIORITY,
};
static void addrconf_type_change(struct net_device *dev, unsigned long event)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 2f11366..dc61b0b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3709,7 +3709,10 @@ static int ip6_route_dev_notify(struct notifier_block
*this,
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct net *net = dev_net(dev);
- if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
+ if (!(dev->flags & IFF_LOOPBACK))
+ return NOTIFY_OK;
+
+ if (event == NETDEV_REGISTER) {
net->ipv6.ip6_null_entry->dst.dev = dev;
net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
@@ -3718,6 +3721,12 @@ static int ip6_route_dev_notify(struct notifier_block
*this,
net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
#endif
+ } else if (event == NETDEV_UNREGISTER) {
+ in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev);
+#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+ in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev);
+ in6_dev_put(net->ipv6.ip6_blk_hole_entry->rt6i_idev);
+#endif
}
return NOTIFY_OK;
@@ -4024,7 +4033,7 @@ static struct pernet_operations ip6_route_net_late_ops = {
static struct notifier_block ip6_route_dev_notifier = {
.notifier_call = ip6_route_dev_notify,
- .priority = 0,
+ .priority = ADDRCONF_NOTIFY_PRIORITY - 10,
};
void __init ip6_route_init_special_entries(void)
