ipv6: sr: fix scheduling in RCU when creating seg6 lwtunnel state
Subsystems (3)
PATCH 1/1 ipv6: sr: fix scheduling in RCU when creating seg6 lwtunnel state
From: David Lebrun <[email protected]>
The seg6_build_state() function is called with RCU read lock held, so we cannot use GFP_KERNEL. This patch uses GFP_ATOMIC instead.
[ 92.770271] =============================
[ 92.770628] WARNING: suspicious RCU usage
[ 92.770921] 4.16.0-rc4+ #12 Not tainted
[ 92.771277] -----------------------------
[ 92.771585] ./include/linux/rcupdate.h:302 Illegal context switch in RCU read-side critical section!
[ 92.772279]
[ 92.772279] other info that might help us debug this:
[ 92.772279]
[ 92.773067]
[ 92.773067] rcu_scheduler_active = 2, debug_locks = 1
[ 92.773514] 2 locks held by ip/2413:
[ 92.773765] #0: (rtnl_mutex){+.+.}, at: [<00000000e5461720>] rtnetlink_rcv_msg+0x441/0x4d0
[ 92.774377] #1: (rcu_read_lock){....}, at: [<00000000df4f161e>] lwtunnel_build_state+0x59/0x210
[ 92.775065]
[ 92.775065] stack backtrace:
[ 92.775371] CPU: 0 PID: 2413 Comm: ip Not tainted 4.16.0-rc4+ #12
[ 92.775791] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1.fc27 04/01/2014
[ 92.776608] Call Trace:
[ 92.776852] dump_stack+0x7d/0xbc
[ 92.777130] __schedule+0x133/0xf00
[ 92.777393] ? unwind_get_return_address_ptr+0x50/0x50
[ 92.777783] ? __sched_text_start+0x8/0x8
[ 92.778073] ? rcu_is_watching+0x19/0x30
[ 92.778383] ? kernel_text_address+0x49/0x60
[ 92.778800] ? __kernel_text_address+0x9/0x30
[ 92.779241] ? unwind_get_return_address+0x29/0x40
[ 92.779727] ? pcpu_alloc+0x102/0x8f0
[ 92.780101] _cond_resched+0x23/0x50
[ 92.780459] __mutex_lock+0xbd/0xad0
[ 92.780818] ? pcpu_alloc+0x102/0x8f0
[ 92.781194] ? seg6_build_state+0x11d/0x240
[ 92.781611] ? save_stack+0x9b/0xb0
[ 92.781965] ? __ww_mutex_wakeup_for_backoff+0xf0/0xf0
[ 92.782480] ? seg6_build_state+0x11d/0x240
[ 92.782925] ? lwtunnel_build_state+0x1bd/0x210
[ 92.783393] ? ip6_route_info_create+0x687/0x1640
[ 92.783846] ? ip6_route_add+0x74/0x110
[ 92.784236] ? inet6_rtm_newroute+0x8a/0xd0--- net/ipv6/seg6_iptunnel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/ipv6/seg6_iptunnel.c +1 -1
@@ -418,7 +418,7 @@ static int seg6_build_state(struct nlattr *nla, slwt =seg6_lwt_lwtunnel (newts); - err =dst_cache_init (&slwt->cache,GFP_KERNEL ); + err =dst_cache_init (&slwt->cache,GFP_ATOMIC );if (err) {kfree (newts);return err;
Signature · 1 line
2.16.2.804.g6dcf76e118-goog
Quoted 17 lines · David Lebrun
From: David Lebrun <[email protected]>
The
seg6_build_state()function is called with RCU read lock held, so we cannot useGFP_KERNEL. This patch usesGFP_ATOMICinstead.Fixes 6c8702c60b886 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")Signed-off-by David Lebrun <[email protected]>--- net/ipv6/seg6_iptunnel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/ipv6/seg6_iptunnel.c +0
-0
net/ipv6/seg6_iptunnel.c static int seg6_build_state(struct nlattr *nla, · line 422
Quoted 6 lines
net/ipv6/seg6_iptunnel.c +1
-1@@ -418,7 +418,7 @@ static int seg6_build_state(struct nlattr *nla, slwt =seg6_lwt_lwtunnel (newts); - err =dst_cache_init (&slwt->cache,GFP_KERNEL ); + err =dst_cache_init (&slwt->cache,GFP_ATOMIC );
This is not the proper fix.
Control path holds RTNL and can sleeep if needed.
RCU should be avoided in lwtunnel_build_state()
Quoted 6 lines · Eric Dumazet
This is not the proper fix.
Control path holds RTNL and can sleeep if needed.
RCU should be avoided in
lwtunnel_build_state()
+Roopa
In lwtunnel_build_state(), the RCU protects the lwtunnel_encap_ops "ops" which is rcu-dereferenced. Moreover, the lwtunnel_state_alloc() function, which is used in all build_state functions, also uses GFP_ATOMIC, so this seemed a proper fix, or at least proper mitigation.
Do you suggest that the lwtunnel_encap_ops can be protected in a different way, not requiring RCU ?
Quoted 5 lines · David Lebrun
Quoted 4 lines · Eric Dumazet
This is not the proper fix.
Control path holds RTNL and can sleeep if needed.
Quoted 8 lines
Quoted 2 lines
RCU should be avoided in
lwtunnel_build_state()+Roopa
In
lwtunnel_build_state(), the RCU protects thelwtunnel_encap_ops"ops" which is rcu-dereferenced. Moreover, thelwtunnel_state_alloc()function, which is used in allbuild_statefunctions, also usesGFP_ATOMIC, so this seemed a proper fix, or at least proper mitigation.Do you suggest that the
lwtunnel_encap_opscan be protected in a different way, not requiring RCU ?
Yes, GFP_ATOMIC might be an 'easy fix' for net tree,
but for the future, GFP_KERNEL allocations make more sense in control path.
Many scripts do not handle errors correctly and simply abort. In case of failure, they might retry (busy poll) in the best scenario, consuming cycles that might be needed to exit from pressure.
GFP_KERNEL allocations provide proper scheduling, they really should be the norm for control path.
David Miller
maintainerFrom: Eric Dumazet <[email protected]>
Date: Tue, 20 Mar 2018 10:20:15 -0700
Quoted 11 lines · David Lebrun
Quoted 8 lines · Eric Dumazet
Quoted 6 lines
This is not the proper fix.
Control path holds RTNL and can sleeep if needed.
RCU should be avoided in
lwtunnel_build_state()
Quoted 8 lines
Quoted 5 lines
+Roopa
In
lwtunnel_build_state(), the RCU protects thelwtunnel_encap_ops"ops" which is rcu-dereferenced. Moreover, thelwtunnel_state_alloc()function, which is used in allbuild_statefunctions, also usesGFP_ATOMIC, so this seemed a proper fix, or at least proper mitigation.Do you suggest that the
lwtunnel_encap_opscan be protected in a different way, not requiring RCU ?Yes,
GFP_ATOMICmight be an 'easy fix' for net tree,
but for the future,GFP_KERNELallocations make more sense in control path.
Agreed.
I'll apply this to net and queue it up for -stable, but long term making this able to use GFP_KERNEL properly is the real way to go about it.