* [PATCH] xfrm: dst_entries_init() per-net dst_ops @ 2015-10-27 16:15 dan.streetman 2015-10-28 13:32 ` Dan Streetman 2015-10-29 11:29 ` [PATCHv2] " Dan Streetman 0 siblings, 2 replies; 11+ messages in thread From: dan.streetman @ 2015-10-27 16:15 UTC (permalink / raw) To: Steffen Klassert Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel, Dan Streetman, Dan Streetman From: Dan Streetman <dan.streetman@canonical.com> The ipv4 and ipv6 xfrms each create a template dst_ops object, and perform dst_entries_init() on the template objects. Then each net namespace has its net.xfrm.xfrm[46]_dst_ops field set to the template values. The problem with that is the dst_ops.pcpuc_entries field is a percpu counter and cannot be used correctly by simply copying it to another object. The result of this is a very subtle bug; changes to the dst entries counter from one net namespace may sometimes get applied to a different net namespace dst entries counter. This is because of how the percpu counter works; it has a main count field as well as a pointer to the percpu variables. Each net namespace maintains its own main count variable, but all point to one set of percpu variables. When any net namespace happens to change one of the percpu variables to outside its small batch range, its count is moved to the net namespace's main count variable. So with multiple net namespaces operating concurrently, the dst_ops entries counter can stray from the actual value that it should be; if counts are consistently moved from one net namespace to another (which my testing showed is likely), then one net namespace winds up with a negative dst_ops count (which is reported as 0) while another winds up with a continually increasing count, eventually reaching its gc_thresh limit, which causes all new traffic on the net namespace to fail with -ENOBUFS. This removes the dst_entries_init (and dst_entries_destroy) call for the template dst_ops objects; their counters will never be used. Instead dst_entries_init is called for each net namespace's dst_ops object, right after copying its values from the template, and dst_entries_destroy is called when the net namespace is removed. Signed-off-by: Dan Streetman <dan.streetman@canonical.com> Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- net/ipv4/xfrm4_policy.c | 5 +++-- net/ipv6/xfrm6_policy.c | 10 ++++------ net/xfrm/xfrm_policy.c | 25 +++++++++++++++++++++++-- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index f2606b9..5f747ee 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -235,6 +235,9 @@ static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } +/* This is used as a template only; the dst_entries counter is not + * initialized for this, but must be on per-net copies of this + */ static struct dst_ops xfrm4_dst_ops = { .family = AF_INET, .gc = xfrm4_garbage_collect, @@ -325,8 +328,6 @@ static void __init xfrm4_policy_init(void) void __init xfrm4_init(void) { - dst_entries_init(&xfrm4_dst_ops); - xfrm4_state_init(); xfrm4_policy_init(); xfrm4_protocol_init(); diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index 2cc5840..b895ec1 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -279,6 +279,9 @@ static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } +/* This is used as a template only; the dst_entries counter is not + * initialized for this, but must be on per-net copies of this + */ static struct dst_ops xfrm6_dst_ops = { .family = AF_INET6, .gc = xfrm6_garbage_collect, @@ -376,13 +379,9 @@ int __init xfrm6_init(void) { int ret; - dst_entries_init(&xfrm6_dst_ops); - ret = xfrm6_policy_init(); - if (ret) { - dst_entries_destroy(&xfrm6_dst_ops); + if (ret) goto out; - } ret = xfrm6_state_init(); if (ret) goto out_policy; @@ -411,5 +410,4 @@ void xfrm6_fini(void) xfrm6_protocol_fini(); xfrm6_policy_fini(); xfrm6_state_fini(); - dst_entries_destroy(&xfrm6_dst_ops); } diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 09bfcba..5381719 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2896,12 +2896,32 @@ static void __net_init xfrm_dst_ops_init(struct net *net) rcu_read_lock(); afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); - if (afinfo) + if (afinfo) { net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; + dst_entries_init(&net->xfrm.xfrm4_dst_ops); + } #if IS_ENABLED(CONFIG_IPV6) afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); - if (afinfo) + if (afinfo) { net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; + dst_entries_init(&net->xfrm.xfrm6_dst_ops); + } +#endif + rcu_read_unlock(); +} + +static void xfrm_dst_ops_fini(struct net *net) +{ + struct xfrm_policy_afinfo *afinfo; + + rcu_read_lock(); + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); + if (afinfo) + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); +#if IS_ENABLED(CONFIG_IPV6) + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); + if (afinfo) + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); #endif rcu_read_unlock(); } @@ -3085,6 +3105,7 @@ static void __net_exit xfrm_net_exit(struct net *net) { flow_cache_fini(net); xfrm_sysctl_fini(net); + xfrm_dst_ops_fini(net); xfrm_policy_fini(net); xfrm_state_fini(net); xfrm_statistics_fini(net); -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfrm: dst_entries_init() per-net dst_ops 2015-10-27 16:15 [PATCH] xfrm: dst_entries_init() per-net dst_ops dan.streetman @ 2015-10-28 13:32 ` Dan Streetman 2015-10-28 13:42 ` Hannes Frederic Sowa 2015-10-28 14:07 ` David Miller 2015-10-29 11:29 ` [PATCHv2] " Dan Streetman 1 sibling, 2 replies; 11+ messages in thread From: Dan Streetman @ 2015-10-28 13:32 UTC (permalink / raw) To: Steffen Klassert Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel, Dan Streetman, Dan Streetman On Tue, Oct 27, 2015 at 12:15 PM, <dan.streetman@canonical.com> wrote: > From: Dan Streetman <dan.streetman@canonical.com> > > The ipv4 and ipv6 xfrms each create a template dst_ops object, and > perform dst_entries_init() on the template objects. Then each net > namespace has its net.xfrm.xfrm[46]_dst_ops field set to the template > values. The problem with that is the dst_ops.pcpuc_entries field is > a percpu counter and cannot be used correctly by simply copying it to > another object. > > The result of this is a very subtle bug; changes to the dst entries > counter from one net namespace may sometimes get applied to a different > net namespace dst entries counter. This is because of how the percpu > counter works; it has a main count field as well as a pointer to the > percpu variables. Each net namespace maintains its own main count > variable, but all point to one set of percpu variables. When any net > namespace happens to change one of the percpu variables to outside its > small batch range, its count is moved to the net namespace's main count > variable. So with multiple net namespaces operating concurrently, the > dst_ops entries counter can stray from the actual value that it should > be; if counts are consistently moved from one net namespace to another > (which my testing showed is likely), then one net namespace winds up > with a negative dst_ops count (which is reported as 0) while another > winds up with a continually increasing count, eventually reaching its > gc_thresh limit, which causes all new traffic on the net namespace to > fail with -ENOBUFS. > > This removes the dst_entries_init (and dst_entries_destroy) call for > the template dst_ops objects; their counters will never be used. > Instead dst_entries_init is called for each net namespace's dst_ops > object, right after copying its values from the template, and Well I'm not sure why my test kernel booted, while the test robot found the bug of GFP_KERNEL percpu counter alloc during atomic context. Thanks test robot! I'll update the patch and resend. > dst_entries_destroy is called when the net namespace is removed. > > Signed-off-by: Dan Streetman <dan.streetman@canonical.com> > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > --- > net/ipv4/xfrm4_policy.c | 5 +++-- > net/ipv6/xfrm6_policy.c | 10 ++++------ > net/xfrm/xfrm_policy.c | 25 +++++++++++++++++++++++-- > 3 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c > index f2606b9..5f747ee 100644 > --- a/net/ipv4/xfrm4_policy.c > +++ b/net/ipv4/xfrm4_policy.c > @@ -235,6 +235,9 @@ static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev, > xfrm_dst_ifdown(dst, dev); > } > > +/* This is used as a template only; the dst_entries counter is not > + * initialized for this, but must be on per-net copies of this > + */ > static struct dst_ops xfrm4_dst_ops = { > .family = AF_INET, > .gc = xfrm4_garbage_collect, > @@ -325,8 +328,6 @@ static void __init xfrm4_policy_init(void) > > void __init xfrm4_init(void) > { > - dst_entries_init(&xfrm4_dst_ops); > - > xfrm4_state_init(); > xfrm4_policy_init(); > xfrm4_protocol_init(); > diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c > index 2cc5840..b895ec1 100644 > --- a/net/ipv6/xfrm6_policy.c > +++ b/net/ipv6/xfrm6_policy.c > @@ -279,6 +279,9 @@ static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev, > xfrm_dst_ifdown(dst, dev); > } > > +/* This is used as a template only; the dst_entries counter is not > + * initialized for this, but must be on per-net copies of this > + */ > static struct dst_ops xfrm6_dst_ops = { > .family = AF_INET6, > .gc = xfrm6_garbage_collect, > @@ -376,13 +379,9 @@ int __init xfrm6_init(void) > { > int ret; > > - dst_entries_init(&xfrm6_dst_ops); > - > ret = xfrm6_policy_init(); > - if (ret) { > - dst_entries_destroy(&xfrm6_dst_ops); > + if (ret) > goto out; > - } > ret = xfrm6_state_init(); > if (ret) > goto out_policy; > @@ -411,5 +410,4 @@ void xfrm6_fini(void) > xfrm6_protocol_fini(); > xfrm6_policy_fini(); > xfrm6_state_fini(); > - dst_entries_destroy(&xfrm6_dst_ops); > } > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 09bfcba..5381719 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -2896,12 +2896,32 @@ static void __net_init xfrm_dst_ops_init(struct net *net) > > rcu_read_lock(); > afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); > - if (afinfo) > + if (afinfo) { > net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; > + dst_entries_init(&net->xfrm.xfrm4_dst_ops); > + } > #if IS_ENABLED(CONFIG_IPV6) > afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); > - if (afinfo) > + if (afinfo) { > net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; > + dst_entries_init(&net->xfrm.xfrm6_dst_ops); > + } > +#endif > + rcu_read_unlock(); > +} > + > +static void xfrm_dst_ops_fini(struct net *net) > +{ > + struct xfrm_policy_afinfo *afinfo; > + > + rcu_read_lock(); > + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); > + if (afinfo) > + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); > +#if IS_ENABLED(CONFIG_IPV6) > + afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); > + if (afinfo) > + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); > #endif > rcu_read_unlock(); > } > @@ -3085,6 +3105,7 @@ static void __net_exit xfrm_net_exit(struct net *net) > { > flow_cache_fini(net); > xfrm_sysctl_fini(net); > + xfrm_dst_ops_fini(net); > xfrm_policy_fini(net); > xfrm_state_fini(net); > xfrm_statistics_fini(net); > -- > 2.5.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfrm: dst_entries_init() per-net dst_ops 2015-10-28 13:32 ` Dan Streetman @ 2015-10-28 13:42 ` Hannes Frederic Sowa 2015-10-28 15:09 ` Dan Streetman 2015-10-28 14:07 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: Hannes Frederic Sowa @ 2015-10-28 13:42 UTC (permalink / raw) To: Dan Streetman, Steffen Klassert Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel, Dan Streetman Hello, On Wed, Oct 28, 2015, at 14:32, Dan Streetman wrote: > On Tue, Oct 27, 2015 at 12:15 PM, <dan.streetman@canonical.com> wrote: > > From: Dan Streetman <dan.streetman@canonical.com> > > > > The ipv4 and ipv6 xfrms each create a template dst_ops object, and > > perform dst_entries_init() on the template objects. Then each net > > namespace has its net.xfrm.xfrm[46]_dst_ops field set to the template > > values. The problem with that is the dst_ops.pcpuc_entries field is > > a percpu counter and cannot be used correctly by simply copying it to > > another object. How hard would it be to split of the counters from the dst_ops struct? We could make dst_ops instances const and have normal pointers to them and keep the dst_entries as a small array in net namespace? Bye, Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfrm: dst_entries_init() per-net dst_ops 2015-10-28 13:42 ` Hannes Frederic Sowa @ 2015-10-28 15:09 ` Dan Streetman 0 siblings, 0 replies; 11+ messages in thread From: Dan Streetman @ 2015-10-28 15:09 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Steffen Klassert, Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel, Dan Streetman On Wed, Oct 28, 2015 at 9:42 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > Hello, > > On Wed, Oct 28, 2015, at 14:32, Dan Streetman wrote: >> On Tue, Oct 27, 2015 at 12:15 PM, <dan.streetman@canonical.com> wrote: >> > From: Dan Streetman <dan.streetman@canonical.com> >> > >> > The ipv4 and ipv6 xfrms each create a template dst_ops object, and >> > perform dst_entries_init() on the template objects. Then each net >> > namespace has its net.xfrm.xfrm[46]_dst_ops field set to the template >> > values. The problem with that is the dst_ops.pcpuc_entries field is >> > a percpu counter and cannot be used correctly by simply copying it to >> > another object. > > How hard would it be to split of the counters from the dst_ops struct? > We could make dst_ops instances const and have normal pointers to them > and keep the dst_entries as a small array in net namespace? Well, the dst_ops->pcpuc_entries counter is used in dst.c which just gets a struct dst_ops *, so it doesn't have access to the owning net namespace. And, not all dst_ops users have a per-net-namespace dst_ops; ipv4/route.c for example uses a global "ipv4_dst_ops" object. So it probably does need to stay owned by dst_ops. > > Bye, > Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfrm: dst_entries_init() per-net dst_ops 2015-10-28 13:32 ` Dan Streetman 2015-10-28 13:42 ` Hannes Frederic Sowa @ 2015-10-28 14:07 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: David Miller @ 2015-10-28 14:07 UTC (permalink / raw) To: dan.streetman Cc: steffen.klassert, herbert, kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel, ddstreet From: Dan Streetman <dan.streetman@canonical.com> Date: Wed, 28 Oct 2015 09:32:47 -0400 > Well I'm not sure why my test kernel booted, while the test robot > found the bug of GFP_KERNEL percpu counter alloc during atomic > context. Thanks test robot! It's because of the kernel config options you (don't) have enabled. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2] xfrm: dst_entries_init() per-net dst_ops 2015-10-27 16:15 [PATCH] xfrm: dst_entries_init() per-net dst_ops dan.streetman 2015-10-28 13:32 ` Dan Streetman @ 2015-10-29 11:29 ` Dan Streetman 2015-10-29 11:57 ` kbuild test robot 2015-10-29 13:51 ` [PATCHv3] " Dan Streetman 1 sibling, 2 replies; 11+ messages in thread From: Dan Streetman @ 2015-10-29 11:29 UTC (permalink / raw) To: Steffen Klassert Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev, linux-kernel, Dan Streetman, Dan Streetman Remove the dst_entries_init/destroy calls for xfrm4 and xfrm6 dst_ops templates; their dst_entries counters will never be used. Move the xfrm dst_ops initialization from the common xfrm/xfrm_policy.c to xfrm4/xfrm4_policy.c and xfrm6/xfrm6_policy.c, and call dst_entries_init and dst_entries_destroy for each net namespace. The ipv4 and ipv6 xfrms each create dst_ops template, and perform dst_entries_init on the templates. The template values are copied to each net namespace's xfrm.xfrm*_dst_ops. The problem there is the dst_ops pcpuc_entries field is a percpu counter and cannot be used correctly by simply copying it to another object. The result of this is a very subtle bug; changes to the dst entries counter from one net namespace may sometimes get applied to a different net namespace dst entries counter. This is because of how the percpu counter works; it has a main count field as well as a pointer to the percpu variables. Each net namespace maintains its own main count variable, but all point to one set of percpu variables. When any net namespace happens to change one of the percpu variables to outside its small batch range, its count is moved to the net namespace's main count variable. So with multiple net namespaces operating concurrently, the dst_ops entries counter can stray from the actual value that it should be; if counts are consistently moved from one net namespace to another (which my testing showed is likely), then one net namespace winds up with a negative dst_ops count while another winds up with a continually increasing count, eventually reaching its gc_thresh limit, which causes all new traffic on the net namespace to fail with -ENOBUFS. Signed-off-by: Dan Streetman <dan.streetman@canonical.com> Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- Changes since v1: move dst copying and entries_init from xfrm/xfrm_policy into ipv[46]/xfrm[46]_policy add dst_entries_init and destroy to xfrm[46]_policy pernet init/exit, so no for_each_net() is required net/ipv4/xfrm4_policy.c | 46 +++++++++++++++++++++++++++++++++--------- net/ipv6/xfrm6_policy.c | 53 +++++++++++++++++++++++++++++++++++-------------- net/xfrm/xfrm_policy.c | 37 ---------------------------------- 3 files changed, 75 insertions(+), 61 deletions(-) diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index f2606b9..59ce2b9 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -235,7 +235,7 @@ static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm4_dst_ops = { +static struct dst_ops xfrm4_dst_ops_template = { .family = AF_INET, .gc = xfrm4_garbage_collect, .update_pmtu = xfrm4_update_pmtu, @@ -249,7 +249,7 @@ static struct dst_ops xfrm4_dst_ops = { static struct xfrm_policy_afinfo xfrm4_policy_afinfo = { .family = AF_INET, - .dst_ops = &xfrm4_dst_ops, + .dst_ops = &xfrm4_dst_ops_template, .dst_lookup = xfrm4_dst_lookup, .get_saddr = xfrm4_get_saddr, .decode_session = _decode_session4, @@ -271,7 +271,7 @@ static struct ctl_table xfrm4_policy_table[] = { { } }; -static int __net_init xfrm4_net_init(struct net *net) +static int __net_init xfrm4_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -299,7 +299,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm4_net_exit(struct net *net) +static void __net_exit xfrm4_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -311,12 +311,44 @@ static void __net_exit xfrm4_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm4_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm4_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm4_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm4_dst_ops, &xfrm4_dst_ops_template, + sizeof(xfrm4_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm4_dst_ops); + if (ret) + return ret; + + ret = xfrm4_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); + + return ret; +} + +static void __net_exit xfrm4_net_exit(struct net *net) +{ + xfrm4_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); +} static struct pernet_operations __net_initdata xfrm4_net_ops = { .init = xfrm4_net_init, .exit = xfrm4_net_exit, }; -#endif static void __init xfrm4_policy_init(void) { @@ -325,13 +357,9 @@ static void __init xfrm4_policy_init(void) void __init xfrm4_init(void) { - dst_entries_init(&xfrm4_dst_ops); - xfrm4_state_init(); xfrm4_policy_init(); xfrm4_protocol_init(); -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm4_net_ops); -#endif } diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index 2cc5840..7b0bbb8 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -279,7 +279,7 @@ static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm6_dst_ops = { +static struct dst_ops xfrm6_dst_ops_template = { .family = AF_INET6, .gc = xfrm6_garbage_collect, .update_pmtu = xfrm6_update_pmtu, @@ -293,7 +293,7 @@ static struct dst_ops xfrm6_dst_ops = { static struct xfrm_policy_afinfo xfrm6_policy_afinfo = { .family = AF_INET6, - .dst_ops = &xfrm6_dst_ops, + .dst_ops = &xfrm6_dst_ops_template, .dst_lookup = xfrm6_dst_lookup, .get_saddr = xfrm6_get_saddr, .decode_session = _decode_session6, @@ -325,7 +325,7 @@ static struct ctl_table xfrm6_policy_table[] = { { } }; -static int __net_init xfrm6_net_init(struct net *net) +static int __net_init xfrm6_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -353,7 +353,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm6_net_exit(struct net *net) +static void __net_exit xfrm6_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -365,24 +365,52 @@ static void __net_exit xfrm6_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm6_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm6_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm6_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm6_dst_ops, &xfrm6_dst_ops_template, + sizeof(xfrm6_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm6_dst_ops); + if (ret) + return ret; + + ret = xfrm6_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); + + return ret; +} + +static void __net_exit xfrm6_net_exit(struct net *net) +{ + xfrm6_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); +} static struct pernet_operations xfrm6_net_ops = { .init = xfrm6_net_init, .exit = xfrm6_net_exit, }; -#endif int __init xfrm6_init(void) { int ret; - dst_entries_init(&xfrm6_dst_ops); - ret = xfrm6_policy_init(); - if (ret) { - dst_entries_destroy(&xfrm6_dst_ops); + if (ret) goto out; - } ret = xfrm6_state_init(); if (ret) goto out_policy; @@ -391,9 +419,7 @@ int __init xfrm6_init(void) if (ret) goto out_state; -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm6_net_ops); -#endif out: return ret; out_state: @@ -405,11 +431,8 @@ out_policy: void xfrm6_fini(void) { -#ifdef CONFIG_SYSCTL unregister_pernet_subsys(&xfrm6_net_ops); -#endif xfrm6_protocol_fini(); xfrm6_policy_fini(); xfrm6_state_fini(); - dst_entries_destroy(&xfrm6_dst_ops); } diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 09bfcba..836229c 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2835,26 +2835,6 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) } spin_unlock(&xfrm_policy_afinfo_lock); - rtnl_lock(); - for_each_net(net) { - struct dst_ops *xfrm_dst_ops; - - switch (afinfo->family) { - case AF_INET: - xfrm_dst_ops = &net->xfrm.xfrm4_dst_ops; - break; -#if IS_ENABLED(CONFIG_IPV6) - case AF_INET6: - xfrm_dst_ops = &net->xfrm.xfrm6_dst_ops; - break; -#endif - default: - BUG(); - } - *xfrm_dst_ops = *afinfo->dst_ops; - } - rtnl_unlock(); - return err; } EXPORT_SYMBOL(xfrm_policy_register_afinfo); @@ -2890,22 +2870,6 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo) } EXPORT_SYMBOL(xfrm_policy_unregister_afinfo); -static void __net_init xfrm_dst_ops_init(struct net *net) -{ - struct xfrm_policy_afinfo *afinfo; - - rcu_read_lock(); - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); - if (afinfo) - net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; -#if IS_ENABLED(CONFIG_IPV6) - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); - if (afinfo) - net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; -#endif - rcu_read_unlock(); -} - static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); @@ -3054,7 +3018,6 @@ static int __net_init xfrm_net_init(struct net *net) rv = xfrm_policy_init(net); if (rv < 0) goto out_policy; - xfrm_dst_ops_init(net); rv = xfrm_sysctl_init(net); if (rv < 0) goto out_sysctl; -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2] xfrm: dst_entries_init() per-net dst_ops 2015-10-29 11:29 ` [PATCHv2] " Dan Streetman @ 2015-10-29 11:57 ` kbuild test robot 2015-10-29 13:46 ` Dan Streetman 2015-10-29 13:51 ` [PATCHv3] " Dan Streetman 1 sibling, 1 reply; 11+ messages in thread From: kbuild test robot @ 2015-10-29 11:57 UTC (permalink / raw) To: Dan Streetman Cc: kbuild-all, Steffen Klassert, Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev, linux-kernel, Dan Streetman, Dan Streetman [-- Attachment #1: Type: text/plain, Size: 2896 bytes --] Hi Dan, [auto build test WARNING on ipsec/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Dan-Streetman/xfrm-dst_entries_init-per-net-dst_ops/20151029-193245 config: x86_64-randconfig-x019-201543 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): net/xfrm/xfrm_policy.c: In function 'xfrm_policy_register_afinfo': >> net/xfrm/xfrm_policy.c:2810:14: warning: unused variable 'net' [-Wunused-variable] struct net *net; ^ vim +/net +2810 net/xfrm/xfrm_policy.c ebb762f27 Steffen Klassert 2011-11-23 2794 static unsigned int xfrm_mtu(const struct dst_entry *dst) d33e45533 David S. Miller 2010-12-14 2795 { 618f9bc74 Steffen Klassert 2011-11-23 2796 unsigned int mtu = dst_metric_raw(dst, RTAX_MTU); 618f9bc74 Steffen Klassert 2011-11-23 2797 618f9bc74 Steffen Klassert 2011-11-23 2798 return mtu ? : dst_mtu(dst->path); d33e45533 David S. Miller 2010-12-14 2799 } d33e45533 David S. Miller 2010-12-14 2800 f894cbf84 David S. Miller 2012-07-02 2801 static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst, f894cbf84 David S. Miller 2012-07-02 2802 struct sk_buff *skb, f894cbf84 David S. Miller 2012-07-02 2803 const void *daddr) d3aaeb38c David S. Miller 2011-07-18 2804 { f894cbf84 David S. Miller 2012-07-02 2805 return dst->path->ops->neigh_lookup(dst, skb, daddr); d3aaeb38c David S. Miller 2011-07-18 2806 } d3aaeb38c David S. Miller 2011-07-18 2807 ^1da177e4 Linus Torvalds 2005-04-16 2808 int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) ^1da177e4 Linus Torvalds 2005-04-16 2809 { d7c7544c3 Alexey Dobriyan 2010-01-24 @2810 struct net *net; ^1da177e4 Linus Torvalds 2005-04-16 2811 int err = 0; ^1da177e4 Linus Torvalds 2005-04-16 2812 if (unlikely(afinfo == NULL)) ^1da177e4 Linus Torvalds 2005-04-16 2813 return -EINVAL; ^1da177e4 Linus Torvalds 2005-04-16 2814 if (unlikely(afinfo->family >= NPROTO)) ^1da177e4 Linus Torvalds 2005-04-16 2815 return -EAFNOSUPPORT; ef8531b64 Eric Dumazet 2012-08-19 2816 spin_lock(&xfrm_policy_afinfo_lock); ^1da177e4 Linus Torvalds 2005-04-16 2817 if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL)) f31e8d4f7 Li RongQing 2015-04-23 2818 err = -EEXIST; :::::: The code at line 2810 was first introduced by commit :::::: d7c7544c3d5f59033d1bf3236bc7b289f5f26b75 netns xfrm: deal with dst entries in netns :::::: TO: Alexey Dobriyan <adobriyan@gmail.com> :::::: CC: David S. Miller <davem@davemloft.net> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 25550 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] xfrm: dst_entries_init() per-net dst_ops 2015-10-29 11:57 ` kbuild test robot @ 2015-10-29 13:46 ` Dan Streetman 0 siblings, 0 replies; 11+ messages in thread From: Dan Streetman @ 2015-10-29 13:46 UTC (permalink / raw) To: kbuild test robot Cc: kbuild-all, Steffen Klassert, Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev, linux-kernel, Dan Streetman On Thu, Oct 29, 2015 at 7:57 AM, kbuild test robot <lkp@intel.com> wrote: > Hi Dan, > > [auto build test WARNING on ipsec/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base] > > url: https://github.com/0day-ci/linux/commits/Dan-Streetman/xfrm-dst_entries_init-per-net-dst_ops/20151029-193245 > config: x86_64-randconfig-x019-201543 (attached as .config) > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All warnings (new ones prefixed by >>): > > net/xfrm/xfrm_policy.c: In function 'xfrm_policy_register_afinfo': >>> net/xfrm/xfrm_policy.c:2810:14: warning: unused variable 'net' [-Wunused-variable] > struct net *net; > ^ well, i should have caught that one. resending patch. thnx! > > vim +/net +2810 net/xfrm/xfrm_policy.c > > ebb762f27 Steffen Klassert 2011-11-23 2794 static unsigned int xfrm_mtu(const struct dst_entry *dst) > d33e45533 David S. Miller 2010-12-14 2795 { > 618f9bc74 Steffen Klassert 2011-11-23 2796 unsigned int mtu = dst_metric_raw(dst, RTAX_MTU); > 618f9bc74 Steffen Klassert 2011-11-23 2797 > 618f9bc74 Steffen Klassert 2011-11-23 2798 return mtu ? : dst_mtu(dst->path); > d33e45533 David S. Miller 2010-12-14 2799 } > d33e45533 David S. Miller 2010-12-14 2800 > f894cbf84 David S. Miller 2012-07-02 2801 static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst, > f894cbf84 David S. Miller 2012-07-02 2802 struct sk_buff *skb, > f894cbf84 David S. Miller 2012-07-02 2803 const void *daddr) > d3aaeb38c David S. Miller 2011-07-18 2804 { > f894cbf84 David S. Miller 2012-07-02 2805 return dst->path->ops->neigh_lookup(dst, skb, daddr); > d3aaeb38c David S. Miller 2011-07-18 2806 } > d3aaeb38c David S. Miller 2011-07-18 2807 > ^1da177e4 Linus Torvalds 2005-04-16 2808 int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) > ^1da177e4 Linus Torvalds 2005-04-16 2809 { > d7c7544c3 Alexey Dobriyan 2010-01-24 @2810 struct net *net; > ^1da177e4 Linus Torvalds 2005-04-16 2811 int err = 0; > ^1da177e4 Linus Torvalds 2005-04-16 2812 if (unlikely(afinfo == NULL)) > ^1da177e4 Linus Torvalds 2005-04-16 2813 return -EINVAL; > ^1da177e4 Linus Torvalds 2005-04-16 2814 if (unlikely(afinfo->family >= NPROTO)) > ^1da177e4 Linus Torvalds 2005-04-16 2815 return -EAFNOSUPPORT; > ef8531b64 Eric Dumazet 2012-08-19 2816 spin_lock(&xfrm_policy_afinfo_lock); > ^1da177e4 Linus Torvalds 2005-04-16 2817 if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL)) > f31e8d4f7 Li RongQing 2015-04-23 2818 err = -EEXIST; > > :::::: The code at line 2810 was first introduced by commit > :::::: d7c7544c3d5f59033d1bf3236bc7b289f5f26b75 netns xfrm: deal with dst entries in netns > > :::::: TO: Alexey Dobriyan <adobriyan@gmail.com> > :::::: CC: David S. Miller <davem@davemloft.net> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv3] xfrm: dst_entries_init() per-net dst_ops 2015-10-29 11:29 ` [PATCHv2] " Dan Streetman 2015-10-29 11:57 ` kbuild test robot @ 2015-10-29 13:51 ` Dan Streetman 2015-11-03 13:22 ` Steffen Klassert 1 sibling, 1 reply; 11+ messages in thread From: Dan Streetman @ 2015-10-29 13:51 UTC (permalink / raw) To: Steffen Klassert Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev, linux-kernel, Dan Streetman, Dan Streetman Remove the dst_entries_init/destroy calls for xfrm4 and xfrm6 dst_ops templates; their dst_entries counters will never be used. Move the xfrm dst_ops initialization from the common xfrm/xfrm_policy.c to xfrm4/xfrm4_policy.c and xfrm6/xfrm6_policy.c, and call dst_entries_init and dst_entries_destroy for each net namespace. The ipv4 and ipv6 xfrms each create dst_ops template, and perform dst_entries_init on the templates. The template values are copied to each net namespace's xfrm.xfrm*_dst_ops. The problem there is the dst_ops pcpuc_entries field is a percpu counter and cannot be used correctly by simply copying it to another object. The result of this is a very subtle bug; changes to the dst entries counter from one net namespace may sometimes get applied to a different net namespace dst entries counter. This is because of how the percpu counter works; it has a main count field as well as a pointer to the percpu variables. Each net namespace maintains its own main count variable, but all point to one set of percpu variables. When any net namespace happens to change one of the percpu variables to outside its small batch range, its count is moved to the net namespace's main count variable. So with multiple net namespaces operating concurrently, the dst_ops entries counter can stray from the actual value that it should be; if counts are consistently moved from one net namespace to another (which my testing showed is likely), then one net namespace winds up with a negative dst_ops count while another winds up with a continually increasing count, eventually reaching its gc_thresh limit, which causes all new traffic on the net namespace to fail with -ENOBUFS. Signed-off-by: Dan Streetman <dan.streetman@canonical.com> Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- Changes since v2: remove no longer needed struct net *net stack var Changes since v1: move dst copying and entries_init from xfrm/xfrm_policy into ipv[46]/xfrm[46]_policy add dst_entries_init and destroy to xfrm[46]_policy pernet init/exit, so no for_each_net() is required net/ipv4/xfrm4_policy.c | 46 +++++++++++++++++++++++++++++++++--------- net/ipv6/xfrm6_policy.c | 53 +++++++++++++++++++++++++++++++++++-------------- net/xfrm/xfrm_policy.c | 38 ----------------------------------- 3 files changed, 75 insertions(+), 62 deletions(-) diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index f2606b9..59ce2b9 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -235,7 +235,7 @@ static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm4_dst_ops = { +static struct dst_ops xfrm4_dst_ops_template = { .family = AF_INET, .gc = xfrm4_garbage_collect, .update_pmtu = xfrm4_update_pmtu, @@ -249,7 +249,7 @@ static struct dst_ops xfrm4_dst_ops = { static struct xfrm_policy_afinfo xfrm4_policy_afinfo = { .family = AF_INET, - .dst_ops = &xfrm4_dst_ops, + .dst_ops = &xfrm4_dst_ops_template, .dst_lookup = xfrm4_dst_lookup, .get_saddr = xfrm4_get_saddr, .decode_session = _decode_session4, @@ -271,7 +271,7 @@ static struct ctl_table xfrm4_policy_table[] = { { } }; -static int __net_init xfrm4_net_init(struct net *net) +static int __net_init xfrm4_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -299,7 +299,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm4_net_exit(struct net *net) +static void __net_exit xfrm4_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -311,12 +311,44 @@ static void __net_exit xfrm4_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm4_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm4_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm4_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm4_dst_ops, &xfrm4_dst_ops_template, + sizeof(xfrm4_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm4_dst_ops); + if (ret) + return ret; + + ret = xfrm4_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); + + return ret; +} + +static void __net_exit xfrm4_net_exit(struct net *net) +{ + xfrm4_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); +} static struct pernet_operations __net_initdata xfrm4_net_ops = { .init = xfrm4_net_init, .exit = xfrm4_net_exit, }; -#endif static void __init xfrm4_policy_init(void) { @@ -325,13 +357,9 @@ static void __init xfrm4_policy_init(void) void __init xfrm4_init(void) { - dst_entries_init(&xfrm4_dst_ops); - xfrm4_state_init(); xfrm4_policy_init(); xfrm4_protocol_init(); -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm4_net_ops); -#endif } diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index 2cc5840..9c4c96b 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -279,7 +279,7 @@ static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm6_dst_ops = { +static struct dst_ops xfrm6_dst_ops_template = { .family = AF_INET6, .gc = xfrm6_garbage_collect, .update_pmtu = xfrm6_update_pmtu, @@ -293,7 +293,7 @@ static struct dst_ops xfrm6_dst_ops = { static struct xfrm_policy_afinfo xfrm6_policy_afinfo = { .family = AF_INET6, - .dst_ops = &xfrm6_dst_ops, + .dst_ops = &xfrm6_dst_ops_template, .dst_lookup = xfrm6_dst_lookup, .get_saddr = xfrm6_get_saddr, .decode_session = _decode_session6, @@ -325,7 +325,7 @@ static struct ctl_table xfrm6_policy_table[] = { { } }; -static int __net_init xfrm6_net_init(struct net *net) +static int __net_init xfrm6_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -353,7 +353,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm6_net_exit(struct net *net) +static void __net_exit xfrm6_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -365,24 +365,52 @@ static void __net_exit xfrm6_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm6_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm6_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm6_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm6_dst_ops, &xfrm6_dst_ops_template, + sizeof(xfrm6_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm6_dst_ops); + if (ret) + return ret; + + ret = xfrm6_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); + + return ret; +} + +static void __net_exit xfrm6_net_exit(struct net *net) +{ + xfrm6_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); +} static struct pernet_operations xfrm6_net_ops = { .init = xfrm6_net_init, .exit = xfrm6_net_exit, }; -#endif int __init xfrm6_init(void) { int ret; - dst_entries_init(&xfrm6_dst_ops); - ret = xfrm6_policy_init(); - if (ret) { - dst_entries_destroy(&xfrm6_dst_ops); + if (ret) goto out; - } ret = xfrm6_state_init(); if (ret) goto out_policy; @@ -391,9 +419,7 @@ int __init xfrm6_init(void) if (ret) goto out_state; -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm6_net_ops); -#endif out: return ret; out_state: @@ -405,11 +431,8 @@ out_policy: void xfrm6_fini(void) { -#ifdef CONFIG_SYSCTL unregister_pernet_subsys(&xfrm6_net_ops); -#endif xfrm6_protocol_fini(); xfrm6_policy_fini(); xfrm6_state_fini(); - dst_entries_destroy(&xfrm6_dst_ops); } diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 09bfcba..aed6a84 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2804,7 +2804,6 @@ static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst, int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) { - struct net *net; int err = 0; if (unlikely(afinfo == NULL)) return -EINVAL; @@ -2835,26 +2834,6 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) } spin_unlock(&xfrm_policy_afinfo_lock); - rtnl_lock(); - for_each_net(net) { - struct dst_ops *xfrm_dst_ops; - - switch (afinfo->family) { - case AF_INET: - xfrm_dst_ops = &net->xfrm.xfrm4_dst_ops; - break; -#if IS_ENABLED(CONFIG_IPV6) - case AF_INET6: - xfrm_dst_ops = &net->xfrm.xfrm6_dst_ops; - break; -#endif - default: - BUG(); - } - *xfrm_dst_ops = *afinfo->dst_ops; - } - rtnl_unlock(); - return err; } EXPORT_SYMBOL(xfrm_policy_register_afinfo); @@ -2890,22 +2869,6 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo) } EXPORT_SYMBOL(xfrm_policy_unregister_afinfo); -static void __net_init xfrm_dst_ops_init(struct net *net) -{ - struct xfrm_policy_afinfo *afinfo; - - rcu_read_lock(); - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); - if (afinfo) - net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; -#if IS_ENABLED(CONFIG_IPV6) - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); - if (afinfo) - net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; -#endif - rcu_read_unlock(); -} - static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); @@ -3054,7 +3017,6 @@ static int __net_init xfrm_net_init(struct net *net) rv = xfrm_policy_init(net); if (rv < 0) goto out_policy; - xfrm_dst_ops_init(net); rv = xfrm_sysctl_init(net); if (rv < 0) goto out_sysctl; -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv3] xfrm: dst_entries_init() per-net dst_ops 2015-10-29 13:51 ` [PATCHv3] " Dan Streetman @ 2015-11-03 13:22 ` Steffen Klassert 0 siblings, 0 replies; 11+ messages in thread From: Steffen Klassert @ 2015-11-03 13:22 UTC (permalink / raw) To: Dan Streetman Cc: Herbert Xu, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev, linux-kernel, Dan Streetman On Thu, Oct 29, 2015 at 09:51:16AM -0400, Dan Streetman wrote: > Remove the dst_entries_init/destroy calls for xfrm4 and xfrm6 dst_ops > templates; their dst_entries counters will never be used. Move the > xfrm dst_ops initialization from the common xfrm/xfrm_policy.c to > xfrm4/xfrm4_policy.c and xfrm6/xfrm6_policy.c, and call dst_entries_init > and dst_entries_destroy for each net namespace. > > The ipv4 and ipv6 xfrms each create dst_ops template, and perform > dst_entries_init on the templates. The template values are copied to each > net namespace's xfrm.xfrm*_dst_ops. The problem there is the dst_ops > pcpuc_entries field is a percpu counter and cannot be used correctly by > simply copying it to another object. > > The result of this is a very subtle bug; changes to the dst entries > counter from one net namespace may sometimes get applied to a different > net namespace dst entries counter. This is because of how the percpu > counter works; it has a main count field as well as a pointer to the > percpu variables. Each net namespace maintains its own main count > variable, but all point to one set of percpu variables. When any net > namespace happens to change one of the percpu variables to outside its > small batch range, its count is moved to the net namespace's main count > variable. So with multiple net namespaces operating concurrently, the > dst_ops entries counter can stray from the actual value that it should > be; if counts are consistently moved from one net namespace to another > (which my testing showed is likely), then one net namespace winds up > with a negative dst_ops count while another winds up with a continually > increasing count, eventually reaching its gc_thresh limit, which causes > all new traffic on the net namespace to fail with -ENOBUFS. > > Signed-off-by: Dan Streetman <dan.streetman@canonical.com> > Signed-off-by: Dan Streetman <ddstreet@ieee.org> Applied to the ipsec tree, thanks Dan! ^ permalink raw reply [flat|nested] 11+ messages in thread
* pull request (net): ipsec 2015-12-22
@ 2015-12-22 9:35 Steffen Klassert
2015-12-22 9:35 ` [PATCH] xfrm: dst_entries_init() per-net dst_ops Steffen Klassert
0 siblings, 1 reply; 11+ messages in thread
From: Steffen Klassert @ 2015-12-22 9:35 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev
Just one patch to fix dst_entries_init with multiple namespaces.
>From Dan Streetman.
Please pull or let me know if there are problems.
Thanks!
The following changes since commit 5f6c99e0ab805d8ec9eda105822912d49aa1d409:
net: phy: fix a bug in get_phy_c45_ids (2015-11-02 23:45:20 -0500)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
for you to fetch changes up to a8a572a6b5f2a79280d6e302cb3c1cb1fbaeb3e8:
xfrm: dst_entries_init() per-net dst_ops (2015-11-03 08:42:57 +0100)
----------------------------------------------------------------
Dan Streetman (1):
xfrm: dst_entries_init() per-net dst_ops
net/ipv4/xfrm4_policy.c | 46 +++++++++++++++++++++++++++++++++---------
net/ipv6/xfrm6_policy.c | 53 +++++++++++++++++++++++++++++++++++--------------
net/xfrm/xfrm_policy.c | 38 -----------------------------------
3 files changed, 75 insertions(+), 62 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] xfrm: dst_entries_init() per-net dst_ops 2015-12-22 9:35 pull request (net): ipsec 2015-12-22 Steffen Klassert @ 2015-12-22 9:35 ` Steffen Klassert 0 siblings, 0 replies; 11+ messages in thread From: Steffen Klassert @ 2015-12-22 9:35 UTC (permalink / raw) To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev From: Dan Streetman <dan.streetman@canonical.com> Remove the dst_entries_init/destroy calls for xfrm4 and xfrm6 dst_ops templates; their dst_entries counters will never be used. Move the xfrm dst_ops initialization from the common xfrm/xfrm_policy.c to xfrm4/xfrm4_policy.c and xfrm6/xfrm6_policy.c, and call dst_entries_init and dst_entries_destroy for each net namespace. The ipv4 and ipv6 xfrms each create dst_ops template, and perform dst_entries_init on the templates. The template values are copied to each net namespace's xfrm.xfrm*_dst_ops. The problem there is the dst_ops pcpuc_entries field is a percpu counter and cannot be used correctly by simply copying it to another object. The result of this is a very subtle bug; changes to the dst entries counter from one net namespace may sometimes get applied to a different net namespace dst entries counter. This is because of how the percpu counter works; it has a main count field as well as a pointer to the percpu variables. Each net namespace maintains its own main count variable, but all point to one set of percpu variables. When any net namespace happens to change one of the percpu variables to outside its small batch range, its count is moved to the net namespace's main count variable. So with multiple net namespaces operating concurrently, the dst_ops entries counter can stray from the actual value that it should be; if counts are consistently moved from one net namespace to another (which my testing showed is likely), then one net namespace winds up with a negative dst_ops count while another winds up with a continually increasing count, eventually reaching its gc_thresh limit, which causes all new traffic on the net namespace to fail with -ENOBUFS. Signed-off-by: Dan Streetman <dan.streetman@canonical.com> Signed-off-by: Dan Streetman <ddstreet@ieee.org> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> --- net/ipv4/xfrm4_policy.c | 46 +++++++++++++++++++++++++++++++++--------- net/ipv6/xfrm6_policy.c | 53 +++++++++++++++++++++++++++++++++++-------------- net/xfrm/xfrm_policy.c | 38 ----------------------------------- 3 files changed, 75 insertions(+), 62 deletions(-) diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index c10a9ee..126ff90 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -236,7 +236,7 @@ static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm4_dst_ops = { +static struct dst_ops xfrm4_dst_ops_template = { .family = AF_INET, .gc = xfrm4_garbage_collect, .update_pmtu = xfrm4_update_pmtu, @@ -250,7 +250,7 @@ static struct dst_ops xfrm4_dst_ops = { static struct xfrm_policy_afinfo xfrm4_policy_afinfo = { .family = AF_INET, - .dst_ops = &xfrm4_dst_ops, + .dst_ops = &xfrm4_dst_ops_template, .dst_lookup = xfrm4_dst_lookup, .get_saddr = xfrm4_get_saddr, .decode_session = _decode_session4, @@ -272,7 +272,7 @@ static struct ctl_table xfrm4_policy_table[] = { { } }; -static int __net_init xfrm4_net_init(struct net *net) +static int __net_init xfrm4_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -300,7 +300,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm4_net_exit(struct net *net) +static void __net_exit xfrm4_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -312,12 +312,44 @@ static void __net_exit xfrm4_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm4_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm4_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm4_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm4_dst_ops, &xfrm4_dst_ops_template, + sizeof(xfrm4_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm4_dst_ops); + if (ret) + return ret; + + ret = xfrm4_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); + + return ret; +} + +static void __net_exit xfrm4_net_exit(struct net *net) +{ + xfrm4_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm4_dst_ops); +} static struct pernet_operations __net_initdata xfrm4_net_ops = { .init = xfrm4_net_init, .exit = xfrm4_net_exit, }; -#endif static void __init xfrm4_policy_init(void) { @@ -326,13 +358,9 @@ static void __init xfrm4_policy_init(void) void __init xfrm4_init(void) { - dst_entries_init(&xfrm4_dst_ops); - xfrm4_state_init(); xfrm4_policy_init(); xfrm4_protocol_init(); -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm4_net_ops); -#endif } diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index da55e0c..d51a18d 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -281,7 +281,7 @@ static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev, xfrm_dst_ifdown(dst, dev); } -static struct dst_ops xfrm6_dst_ops = { +static struct dst_ops xfrm6_dst_ops_template = { .family = AF_INET6, .gc = xfrm6_garbage_collect, .update_pmtu = xfrm6_update_pmtu, @@ -295,7 +295,7 @@ static struct dst_ops xfrm6_dst_ops = { static struct xfrm_policy_afinfo xfrm6_policy_afinfo = { .family = AF_INET6, - .dst_ops = &xfrm6_dst_ops, + .dst_ops = &xfrm6_dst_ops_template, .dst_lookup = xfrm6_dst_lookup, .get_saddr = xfrm6_get_saddr, .decode_session = _decode_session6, @@ -327,7 +327,7 @@ static struct ctl_table xfrm6_policy_table[] = { { } }; -static int __net_init xfrm6_net_init(struct net *net) +static int __net_init xfrm6_net_sysctl_init(struct net *net) { struct ctl_table *table; struct ctl_table_header *hdr; @@ -355,7 +355,7 @@ err_alloc: return -ENOMEM; } -static void __net_exit xfrm6_net_exit(struct net *net) +static void __net_exit xfrm6_net_sysctl_exit(struct net *net) { struct ctl_table *table; @@ -367,24 +367,52 @@ static void __net_exit xfrm6_net_exit(struct net *net) if (!net_eq(net, &init_net)) kfree(table); } +#else /* CONFIG_SYSCTL */ +static int inline xfrm6_net_sysctl_init(struct net *net) +{ + return 0; +} + +static void inline xfrm6_net_sysctl_exit(struct net *net) +{ +} +#endif + +static int __net_init xfrm6_net_init(struct net *net) +{ + int ret; + + memcpy(&net->xfrm.xfrm6_dst_ops, &xfrm6_dst_ops_template, + sizeof(xfrm6_dst_ops_template)); + ret = dst_entries_init(&net->xfrm.xfrm6_dst_ops); + if (ret) + return ret; + + ret = xfrm6_net_sysctl_init(net); + if (ret) + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); + + return ret; +} + +static void __net_exit xfrm6_net_exit(struct net *net) +{ + xfrm6_net_sysctl_exit(net); + dst_entries_destroy(&net->xfrm.xfrm6_dst_ops); +} static struct pernet_operations xfrm6_net_ops = { .init = xfrm6_net_init, .exit = xfrm6_net_exit, }; -#endif int __init xfrm6_init(void) { int ret; - dst_entries_init(&xfrm6_dst_ops); - ret = xfrm6_policy_init(); - if (ret) { - dst_entries_destroy(&xfrm6_dst_ops); + if (ret) goto out; - } ret = xfrm6_state_init(); if (ret) goto out_policy; @@ -393,9 +421,7 @@ int __init xfrm6_init(void) if (ret) goto out_state; -#ifdef CONFIG_SYSCTL register_pernet_subsys(&xfrm6_net_ops); -#endif out: return ret; out_state: @@ -407,11 +433,8 @@ out_policy: void xfrm6_fini(void) { -#ifdef CONFIG_SYSCTL unregister_pernet_subsys(&xfrm6_net_ops); -#endif xfrm6_protocol_fini(); xfrm6_policy_fini(); xfrm6_state_fini(); - dst_entries_destroy(&xfrm6_dst_ops); } diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 94af3d0..bacd30b 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2807,7 +2807,6 @@ static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst, int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) { - struct net *net; int err = 0; if (unlikely(afinfo == NULL)) return -EINVAL; @@ -2838,26 +2837,6 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo) } spin_unlock(&xfrm_policy_afinfo_lock); - rtnl_lock(); - for_each_net(net) { - struct dst_ops *xfrm_dst_ops; - - switch (afinfo->family) { - case AF_INET: - xfrm_dst_ops = &net->xfrm.xfrm4_dst_ops; - break; -#if IS_ENABLED(CONFIG_IPV6) - case AF_INET6: - xfrm_dst_ops = &net->xfrm.xfrm6_dst_ops; - break; -#endif - default: - BUG(); - } - *xfrm_dst_ops = *afinfo->dst_ops; - } - rtnl_unlock(); - return err; } EXPORT_SYMBOL(xfrm_policy_register_afinfo); @@ -2893,22 +2872,6 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo) } EXPORT_SYMBOL(xfrm_policy_unregister_afinfo); -static void __net_init xfrm_dst_ops_init(struct net *net) -{ - struct xfrm_policy_afinfo *afinfo; - - rcu_read_lock(); - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]); - if (afinfo) - net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops; -#if IS_ENABLED(CONFIG_IPV6) - afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET6]); - if (afinfo) - net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops; -#endif - rcu_read_unlock(); -} - static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); @@ -3057,7 +3020,6 @@ static int __net_init xfrm_net_init(struct net *net) rv = xfrm_policy_init(net); if (rv < 0) goto out_policy; - xfrm_dst_ops_init(net); rv = xfrm_sysctl_init(net); if (rv < 0) goto out_sysctl; -- 1.9.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-12-22 9:35 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-27 16:15 [PATCH] xfrm: dst_entries_init() per-net dst_ops dan.streetman 2015-10-28 13:32 ` Dan Streetman 2015-10-28 13:42 ` Hannes Frederic Sowa 2015-10-28 15:09 ` Dan Streetman 2015-10-28 14:07 ` David Miller 2015-10-29 11:29 ` [PATCHv2] " Dan Streetman 2015-10-29 11:57 ` kbuild test robot 2015-10-29 13:46 ` Dan Streetman 2015-10-29 13:51 ` [PATCHv3] " Dan Streetman 2015-11-03 13:22 ` Steffen Klassert 2015-12-22 9:35 pull request (net): ipsec 2015-12-22 Steffen Klassert 2015-12-22 9:35 ` [PATCH] xfrm: dst_entries_init() per-net dst_ops Steffen Klassert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.