diff mbox series

net: ipmr: fix suspicious RCU warning

Message ID 20191118090925.2474-1-anders.roxell@linaro.org
State New
Headers show
Series net: ipmr: fix suspicious RCU warning | expand

Commit Message

Anders Roxell Nov. 18, 2019, 9:09 a.m. UTC 
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(-)

-- 
2.20.1

Comments

David Miller Nov. 19, 2019, 10:50 p.m. UTC | #1 
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.
Paul E. McKenney Nov. 20, 2019, 1:12 a.m. UTC | #2 
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.
diff mbox series

Patch

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