[PATCH] net: ipmr: fix suspicious RCU warning

[PATCH] net: ipmr: fix suspicious RCU warning

[Anders Roxell]

When booting an arm64 allmodconfig kernel on linux-next (tag
next-20191115). The following "suspicious RCU usage" warning shows up.
This bug seems to have been introduced by commit f0ad0860d01e ("ipv4:
ipmr: support multiple tables") in 2010, but the warning was added only
in this past year by commit 28875945ba98 ("rcu: Add support for
consolidated-RCU reader checking").

[   32.496021][    T1] =============================
[   32.497616][    T1] WARNING: suspicious RCU usage
[   32.499614][    T1] 5.4.0-rc6-next-20191108-00003-gf74bac957b5c-dirty #2 Not tainted
[   32.502018][    T1] -----------------------------
[   32.503976][    T1] net/ipv4/ipmr.c:136 RCU-list traversed in non-reader section!!
[   32.506746][    T1]
[   32.506746][    T1] other info that might help us debug this:
[   32.506746][    T1]
[   32.509794][    T1]
[   32.509794][    T1] rcu_scheduler_active = 2, debug_locks = 1
[   32.512661][    T1] 1 lock held by swapper/0/1:
[   32.514169][    T1]  #0: ffffa000150dd678 (pernet_ops_rwsem){+.+.}, at: register_pernet_subsys+0x24/0x50
[   32.517621][    T1]
[   32.517621][    T1] stack backtrace:
[   32.519930][    T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.4.0-rc6-next-20191108-00003-gf74bac957b5c-dirty #2
[   32.523063][    T1] Hardware name: linux,dummy-virt (DT)
[   32.524787][    T1] Call trace:
[   32.525946][    T1]  dump_backtrace+0x0/0x2d0
[   32.527433][    T1]  show_stack+0x20/0x30
[   32.528811][    T1]  dump_stack+0x204/0x2ac
[   32.530258][    T1]  lockdep_rcu_suspicious+0xf4/0x108
[   32.531993][    T1]  ipmr_get_table+0xc8/0x170
[   32.533496][    T1]  ipmr_new_table+0x48/0xa0
[   32.535002][    T1]  ipmr_net_init+0xe8/0x258
[   32.536465][    T1]  ops_init+0x280/0x2d8
[   32.537876][    T1]  register_pernet_operations+0x210/0x420
[   32.539707][    T1]  register_pernet_subsys+0x30/0x50
[   32.541372][    T1]  ip_mr_init+0x54/0x180
[   32.542785][    T1]  inet_init+0x25c/0x3e8
[   32.544186][    T1]  do_one_initcall+0x4c0/0xad8
[   32.545757][    T1]  kernel_init_freeable+0x3e0/0x500
[   32.547443][    T1]  kernel_init+0x14/0x1f0
[   32.548875][    T1]  ret_from_fork+0x10/0x18

This commit therefore introduces a lockdep-specific variable that
maintains initialization state.  It then passes this variable along with
the return value of lockdep_rtnl_is_held() to list_for_each_entry_rcu()
in order to correctly check for proper RCU/locking/initialization state.

Suggested-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 net/ipv4/ipmr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6e68def66822..93007c429dae 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -108,9 +108,18 @@ static void igmpmsg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt);
 static void mroute_clean_tables(struct mr_table *mrt, int flags);
 static void ipmr_expire_process(struct timer_list *t);
 
+#ifdef CONFIG_PROVE_LOCKING
+int ip_mr_initialized;
+void ip_mr_now_initialized(void) { ip_mr_initialized = 1; }
+#else
+const int ip_mr_initialized = 1;
+void ip_mr_now_initialized(void) { }
+#endif
+
 #ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
 #define ipmr_for_each_table(mrt, net) \
-	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list)
+	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list, \
+			(lockdep_rtnl_is_held() || !ip_mr_initialized))
 
 static struct mr_table *ipmr_mr_table_iter(struct net *net,
 					   struct mr_table *mrt)
@@ -3160,6 +3169,8 @@ int __init ip_mr_init(void)
 
 	rtnl_register(RTNL_FAMILY_IPMR, RTM_GETLINK,
 		      NULL, ipmr_rtm_dumplink, 0);
+
+	ip_mr_now_initialized();
 	return 0;
 
 #ifdef CONFIG_IP_PIMSM_V2
-- 
2.20.1

Re: [PATCH] net: ipmr: fix suspicious RCU warning

[David Miller]

From: Anders Roxell <anders.roxell@linaro.org>
Date: Mon, 18 Nov 2019 10:09:25 +0100

> @@ -108,9 +108,18 @@ static void igmpmsg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt);
>  static void mroute_clean_tables(struct mr_table *mrt, int flags);
>  static void ipmr_expire_process(struct timer_list *t);
>  
> +#ifdef CONFIG_PROVE_LOCKING
> +int ip_mr_initialized;
> +void ip_mr_now_initialized(void) { ip_mr_initialized = 1; }
> +#else
> +const int ip_mr_initialized = 1;
> +void ip_mr_now_initialized(void) { }
> +#endif

This seems excessive and a bit not so pretty.

> +
>  #ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
>  #define ipmr_for_each_table(mrt, net) \
> -	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list)
> +	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list, \
> +			(lockdep_rtnl_is_held() || !ip_mr_initialized))
>  
>  static struct mr_table *ipmr_mr_table_iter(struct net *net,
>  					   struct mr_table *mrt)

The problematic code path is ipmr_rules_init() done during ipmr_net_init().

You can just wrap this call around RCU locking or take the RTNL mutex.

That way you don't need to rediculous ip_mr_initialized knob which frankly
doesn't even seem accurate to me.  It's a centralized global variable
which is holding state about multiple network namespace objects which makes
absolutely no sense at all, it's wrong.

Re: [PATCH] net: ipmr: fix suspicious RCU warning

[Paul E. McKenney]

On Tue, Nov 19, 2019 at 02:50:48PM -0800, David Miller wrote:
> From: Anders Roxell <anders.roxell@linaro.org>
> Date: Mon, 18 Nov 2019 10:09:25 +0100
> 
> > @@ -108,9 +108,18 @@ static void igmpmsg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt);
> >  static void mroute_clean_tables(struct mr_table *mrt, int flags);
> >  static void ipmr_expire_process(struct timer_list *t);
> >  
> > +#ifdef CONFIG_PROVE_LOCKING
> > +int ip_mr_initialized;
> > +void ip_mr_now_initialized(void) { ip_mr_initialized = 1; }
> > +#else
> > +const int ip_mr_initialized = 1;
> > +void ip_mr_now_initialized(void) { }
> > +#endif
> 
> This seems excessive and a bit not so pretty.
> 
> > +
> >  #ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
> >  #define ipmr_for_each_table(mrt, net) \
> > -	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list)
> > +	list_for_each_entry_rcu(mrt, &net->ipv4.mr_tables, list, \
> > +			(lockdep_rtnl_is_held() || !ip_mr_initialized))
> >  
> >  static struct mr_table *ipmr_mr_table_iter(struct net *net,
> >  					   struct mr_table *mrt)
> 
> The problematic code path is ipmr_rules_init() done during ipmr_net_init().
> 
> You can just wrap this call around RCU locking or take the RTNL mutex.

Agreed, that would work quite well.

							Thanx, Paul

> That way you don't need to rediculous ip_mr_initialized knob which frankly
> doesn't even seem accurate to me.  It's a centralized global variable
> which is holding state about multiple network namespace objects which makes
> absolutely no sense at all, it's wrong.