From: Eric Dumazet <eric.dumazet@gmail.com>
To: Anders Roxell <anders.roxell@linaro.org>,
davem@davemloft.net, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org
Cc: paulmck@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: ipmr: fix suspicious RCU warning
Date: Wed, 20 Nov 2019 09:45:18 -0800 [thread overview]
Message-ID: <e07311c7-24b8-8c48-d6f2-a7c93976613c@gmail.com> (raw)
In-Reply-To: <20191120152255.18928-1-anders.roxell@linaro.org>
On 11/20/19 7:22 AM, Anders Roxell wrote:
> When booting an arm64 allmodconfig kernel on linux-next 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 holds RTNL mutex around the problematic code path,
> which is function ipmr_rules_init() in ipmr_net_init(). This commit
> also adds a lockdep_rtnl_is_held() check to the ipmr_for_each_table()
> macro.
>
> Suggested-by: David Miller <davem@davemloft.net>
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
> net/ipv4/ipmr.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 6e68def66822..53dff9a0e60a 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -110,7 +110,8 @@ static void ipmr_expire_process(struct timer_list *t);
>
> #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())
>
> static struct mr_table *ipmr_mr_table_iter(struct net *net,
> struct mr_table *mrt)
> @@ -3086,7 +3087,9 @@ static int __net_init ipmr_net_init(struct net *net)
> if (err)
> goto ipmr_notifier_fail;
>
> + rtnl_lock();
> err = ipmr_rules_init(net);
> + rtnl_unlock();
> if (err < 0)
> goto ipmr_rules_fail;
Hmmm... this might have performance impact for creation of a new netns
Since the 'struct net' is not yet fully initialized (thus published/visible),
should we really have to grab RTNL (again) only to silence a warning ?
What about the following alternative ?
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 6e68def66822f47fc08d94eddd32a4bd4f9fdfb0..b6dcdce08f1d82c83756a319623e24ae0174092c 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -94,7 +94,7 @@ static DEFINE_SPINLOCK(mfc_unres_lock);
static struct kmem_cache *mrt_cachep __ro_after_init;
-static struct mr_table *ipmr_new_table(struct net *net, u32 id);
+static struct mr_table *ipmr_new_table(struct net *net, u32 id, bool init);
static void ipmr_free_table(struct mr_table *mrt);
static void ip_mr_forward(struct net *net, struct mr_table *mrt,
@@ -245,7 +245,7 @@ static int __net_init ipmr_rules_init(struct net *net)
INIT_LIST_HEAD(&net->ipv4.mr_tables);
- mrt = ipmr_new_table(net, RT_TABLE_DEFAULT);
+ mrt = ipmr_new_table(net, RT_TABLE_DEFAULT, true);
if (IS_ERR(mrt)) {
err = PTR_ERR(mrt);
goto err1;
@@ -322,7 +322,7 @@ static int __net_init ipmr_rules_init(struct net *net)
{
struct mr_table *mrt;
- mrt = ipmr_new_table(net, RT_TABLE_DEFAULT);
+ mrt = ipmr_new_table(net, RT_TABLE_DEFAULT, true);
if (IS_ERR(mrt))
return PTR_ERR(mrt);
net->ipv4.mrt = mrt;
@@ -392,7 +392,7 @@ static struct mr_table_ops ipmr_mr_table_ops = {
.cmparg_any = &ipmr_mr_table_ops_cmparg_any,
};
-static struct mr_table *ipmr_new_table(struct net *net, u32 id)
+static struct mr_table *ipmr_new_table(struct net *net, u32 id, bool init)
{
struct mr_table *mrt;
@@ -400,9 +400,11 @@ static struct mr_table *ipmr_new_table(struct net *net, u32 id)
if (id != RT_TABLE_DEFAULT && id >= 1000000000)
return ERR_PTR(-EINVAL);
- mrt = ipmr_get_table(net, id);
- if (mrt)
- return mrt;
+ if (!init) {
+ mrt = ipmr_get_table(net, id);
+ if (mrt)
+ return mrt;
+ }
return mr_table_alloc(net, id, &ipmr_mr_table_ops,
ipmr_expire_process, ipmr_new_table_set);
@@ -1547,7 +1549,7 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval,
if (sk == rtnl_dereference(mrt->mroute_sk)) {
ret = -EBUSY;
} else {
- mrt = ipmr_new_table(net, uval);
+ mrt = ipmr_new_table(net, uval, false);
if (IS_ERR(mrt))
ret = PTR_ERR(mrt);
else
next prev parent reply other threads:[~2019-11-20 17:45 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-20 15:22 [PATCH v2] net: ipmr: fix suspicious RCU warning Anders Roxell
2019-11-20 17:45 ` Eric Dumazet [this message]
2019-11-21 7:15 ` Anders Roxell
2019-11-21 10:17 ` Anders Roxell
2019-11-21 17:41 ` Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e07311c7-24b8-8c48-d6f2-a7c93976613c@gmail.com \
--to=eric.dumazet@gmail.com \
--cc=anders.roxell@linaro.org \
--cc=davem@davemloft.net \
--cc=kuznet@ms2.inr.ac.ru \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=yoshfuji@linux-ipv6.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).