| Message ID | 20180320144456.223556-1-dav.lebrun@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: sr: fix scheduling in RCU when creating seg6 lwtunnel state | expand |
On 03/20/2018 07:44 AM, David Lebrun wrote: > From: David Lebrun <dlebrun@google.com> > > The seg6_build_state() function is called with RCU read lock held, > so we cannot use GFP_KERNEL. This patch uses GFP_ATOMIC instead. > > > Fixes: 6c8702c60b886 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels") > Signed-off-by: David Lebrun <dlebrun@google.com> > --- > net/ipv6/seg6_iptunnel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c > index bd6cc688bd19..8367b859a934 100644 > --- a/net/ipv6/seg6_iptunnel.c > +++ b/net/ipv6/seg6_iptunnel.c > @@ -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()
On 20/03/18 15:07, Eric Dumazet wrote: > 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 ?
On 03/20/2018 10:11 AM, David Lebrun wrote: > On 20/03/18 15:07, Eric Dumazet wrote: >> 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 ? 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.
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 20 Mar 2018 10:20:15 -0700 > > > On 03/20/2018 10:11 AM, David Lebrun wrote: >> On 20/03/18 15:07, Eric Dumazet wrote: >>> 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 ? > > Yes, GFP_ATOMIC might be an 'easy fix' for net tree, > but for the future, GFP_KERNEL allocations 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.
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index bd6cc688bd19..8367b859a934 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -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;