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