[Patch net v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[Patch net v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[Cong Wang]

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 <dsahern@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@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)
-- 
2.5.5

Re: [Patch net v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[David Ahern]

On 5/4/17 11:36 AM, Cong Wang 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 <dsahern@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---

Commit message needs a tie in to the problem that Andrey reported. It
solves the same problem for namespaces other than init_net.

Acked-by: David Ahern <dsahern@gmail.com>
Tested-by: David Ahern <dsahern@gmail.com>

Re: [Patch net v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Thu, 4 May 2017 13:41:15 -0600

> On 5/4/17 11:36 AM, Cong Wang 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 <dsahern@gmail.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
> 
> Commit message needs a tie in to the problem that Andrey reported. It
> solves the same problem for namespaces other than init_net.

Cong, please update the commit message as David is requesting.

Thanks.

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[jeffy]

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 <dsahern@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: David Ahern <dsahern@gmail.com>
> Tested-by: David Ahern <dsahern@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)
>

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[Cong Wang]

Hello,

On Mon, Jun 19, 2017 at 8:15 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
> 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);

Interesting, I didn't notice this corner-case, because normally
we would hit the one in rollback_registered_many(). Probably
we need to add a check

if (dev->reg_state == NETREG_UNREGISTERING)

in ip6_route_dev_notify(). Can you give it a try?

I guess we probably need to revise other NETDEV_UNREGISTER
handlers too.

I will send a patch tomorrow.

Thanks!

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[jeffy]

Hi Cong Wang,

On 06/20/2017 12:54 PM, Cong Wang wrote:
> Hello,
>
> On Mon, Jun 19, 2017 at 8:15 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
>> 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);
>
> Interesting, I didn't notice this corner-case, because normally
> we would hit the one in rollback_registered_many(). Probably
> we need to add a check
>
> if (dev->reg_state == NETREG_UNREGISTERING)
>
> in ip6_route_dev_notify(). Can you give it a try?
the NETREG_UNREGISTERING check works for my test:)

but i saw dev_change_net_namespace also call NETDEV_UNREGISTER & 
NETDEV_REGISTER:

int dev_change_net_namespace(struct net_device *dev, struct net *net, 
const char *pat)
{
...
         /* Don't allow namespace local devices to be moved. */
         err = -EINVAL;
         if (dev->features & NETIF_F_NETNS_LOCAL)
                 goto out;

         /* Ensure the device has been registrered */
         if (dev->reg_state != NETREG_REGISTERED)
                 goto out;
...
         /* Notify protocols, that we are about to destroy
          * this device. They should clean all the things.
          *
          * Note that dev->reg_state stays at NETREG_REGISTERED.
          * This is wanted because this way 8021q and macvlan know
          * the device is just moving and can keep their slaves up.
          */
         call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
...
         /* Notify protocols, that a new device appeared. */
         call_netdevice_notifiers(NETDEV_REGISTER, dev);


maybe we should also add a check for NETDEV_REGISTER event? maybe:
         if (dev->reg_state != NETREG_REGISTERED)


>
> I guess we probably need to revise other NETDEV_UNREGISTER
> handlers too.
>
> I will send a patch tomorrow.
sounds great~
>
> Thanks!
>
>
>

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

[Cong Wang]

On Mon, Jun 19, 2017 at 11:37 PM, jeffy <jeffy.chen@rock-chips.com> wrote:
> Hi Cong Wang,
>
>
> On 06/20/2017 12:54 PM, Cong Wang wrote:
>>
>> Interesting, I didn't notice this corner-case, because normally
>> we would hit the one in rollback_registered_many(). Probably
>> we need to add a check
>>
>> if (dev->reg_state == NETREG_UNREGISTERING)
>>
>> in ip6_route_dev_notify(). Can you give it a try?
>
> the NETREG_UNREGISTERING check works for my test:)
>
> but i saw dev_change_net_namespace also call NETDEV_UNREGISTER &
> NETDEV_REGISTER:

Yes we should call it in this case too, only netdev_wait_allrefs()
is an exceptional case here.

I just sent out a formal patch with you Cc'ed.

Thanks!