[PATCH net v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()

Stefan Wiehler posted 10 patches 1 month, 1 week ago
[PATCH net v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()
Posted by Stefan Wiehler 1 month, 1 week ago
When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read under RCU or RTNL lock. Since mr_table_alloc() can sleep,
we call ip6mr_new_table() under the RTNL lock.

Detected by Lockdep-RCU:

  [   10.247131] WARNING: suspicious RCU usage
  [   10.247133] 6.1.103-49518b10de-nokia_sm_x86 #1 Not tainted
  [   10.247135] -----------------------------
  [   10.247137] /net/ipv6/ip6mr.c:131 RCU-list traversed in non-reader section!!
  [   10.247140]
                 other info that might help us debug this:

  [   10.247142]
                 rcu_scheduler_active = 2, debug_locks = 1
  [   10.247144] 1 lock held by swapper/0/1:
  [   10.247147]  #0: ffffffff82b374d0 (pernet_ops_rwsem){+.+.}-{3:3}, at: register_pernet_subsys+0x15/0x40
  [   10.247164]
                 stack backtrace:
  [   10.247166] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.103-49518b10de-nokia_sm_x86 #1
  [   10.247170] Hardware name: Nokia Asil/Default string, BIOS 0ACNA114 07/18/2024
  [   10.247175] Call Trace:
  [   10.247178]  <TASK>
  [   10.247181]  dump_stack_lvl+0xb7/0xe9
  [   10.247189]  lockdep_rcu_suspicious.cold+0x2d/0x64
  [   10.247198]  ip6mr_get_table+0x8a/0x90
  [   10.247203]  ip6mr_net_init+0x7c/0x200
  [   10.247209]  ops_init+0x37/0x1f0
  [   10.247215]  register_pernet_operations+0x129/0x230
  [   10.247221]  ? af_unix_init+0xca/0xca
  [   10.247227]  register_pernet_subsys+0x24/0x40
  [   10.247231]  ip6_mr_init+0x42/0xf2
  [   10.247235]  inet6_init+0x133/0x3b9
  [   10.247238]  do_one_initcall+0x74/0x290
  [   10.247247]  kernel_init_freeable+0x251/0x294
  [   10.247253]  ? rest_init+0x174/0x174
  [   10.247257]  kernel_init+0x16/0x12c
  [   10.247260]  ret_from_fork+0x1f/0x30
  [   10.247271]  </TASK>

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
 net/ipv6/ip6mr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 017f9e31edfb..9bf42aafc7f0 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -236,11 +236,13 @@ static int __net_init ip6mr_rules_init(struct net *net)
 
 	INIT_LIST_HEAD(&net->ipv6.mr6_tables);
 
+	rtnl_lock();
 	mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
 	if (IS_ERR(mrt)) {
 		err = PTR_ERR(mrt);
 		goto err1;
 	}
+	rtnl_unlock();
 
 	err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
 	if (err < 0)
@@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
 	ip6mr_free_table(mrt);
 	rtnl_unlock();
 err1:
+	rtnl_unlock();
 	fib_rules_unregister(ops);
 	return err;
 }
-- 
2.42.0
Re: [PATCH net v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()
Posted by Florian Westphal 1 month, 1 week ago
Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
> +	rtnl_lock();
>  	mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
>  	if (IS_ERR(mrt)) {
>  		err = PTR_ERR(mrt);
>  		goto err1;
>  	}
> +	rtnl_unlock();
>  
>  	err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
>  	if (err < 0)
> @@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
>  	ip6mr_free_table(mrt);
>  	rtnl_unlock();
>  err1:
> +	rtnl_unlock();

Looks like a double-unlock?
Re: [PATCH net v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()
Posted by Stefan Wiehler 1 month, 1 week ago
>> +     rtnl_lock();
>>       mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
>>       if (IS_ERR(mrt)) {
>>               err = PTR_ERR(mrt);
>>               goto err1;
>>       }
>> +     rtnl_unlock();
>>
>>       err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
>>       if (err < 0)
>> @@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
>>       ip6mr_free_table(mrt);
>>       rtnl_unlock();
>>  err1:
>> +     rtnl_unlock();
> 
> Looks like a double-unlock?

Thanks, will fix in v7.