* [PATCH ipsec] net: xfrm_policy: fix device unregistration hang
@ 2018-02-12 17:21 Eyal Birger
2018-02-12 17:55 ` Shannon Nelson
0 siblings, 1 reply; 3+ messages in thread
From: Eyal Birger @ 2018-02-12 17:21 UTC (permalink / raw)
To: steffen.klassert; +Cc: herbert, davem, netdev, fw, shmulik, Eyal Birger
In setups like the following:
Host A -- Host B
tun0 -- ipsec -- eth0 -- eth0 -- ipsec -- tun0
where tun0 are tunnel devices using dst_cache (ipip, ipip6, etc...).
Unregistration of an underlying eth0 device leads to the following log
messages:
unregister_netdevice: waiting for eth0 to become free. Usage count = 2
This is since xfrm dsts device references are not released upon device
unregistration when the xfrm dst is cached in a dst_cache.
Several approaches for fixing this were discussed in [1]; this commit keeps
track of allocated xdsts and releases their device references on a netdev
unregister event.
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
[1] https://patchwork.ozlabs.org/patch/869025/
---
include/net/xfrm.h | 10 ++-----
net/xfrm/xfrm_policy.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+), 7 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 7d20776..c1c8493 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -994,6 +994,8 @@ struct xfrm_dst {
u32 child_mtu_cached;
u32 route_cookie;
u32 path_cookie;
+ struct list_head xfrm_dst_gc;
+ struct xfrm_dst_list *xfrm_dst_gc_list;
};
static inline struct dst_entry *xfrm_dst_path(const struct dst_entry *dst)
@@ -1025,13 +1027,7 @@ static inline void xfrm_dst_set_child(struct xfrm_dst *xdst, struct dst_entry *c
xdst->child = child;
}
-static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
-{
- xfrm_pols_put(xdst->pols, xdst->num_pols);
- dst_release(xdst->route);
- if (likely(xdst->u.dst.xfrm))
- xfrm_state_put(xdst->u.dst.xfrm);
-}
+void xfrm_dst_destroy(struct xfrm_dst *xdst);
#endif
void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev);
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 7a23078..1da8a65 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -45,6 +45,12 @@ struct xfrm_flo {
u8 flags;
};
+struct xfrm_dst_list {
+ spinlock_t lock; /* sync writers */
+ struct list_head head;
+};
+
+static DEFINE_PER_CPU_ALIGNED(struct xfrm_dst_list, xfrm_dst_gc_list);
static DEFINE_PER_CPU(struct xfrm_dst *, xfrm_last_dst);
static struct work_struct *xfrm_pcpu_work __read_mostly;
static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
@@ -94,6 +100,51 @@ __xfrm6_selector_match(const struct xfrm_selector *sel, const struct flowi *fl)
(fl6->flowi6_oif == sel->ifindex || !sel->ifindex);
}
+static void xfrm_dst_add_to_gc_list(struct xfrm_dst *xdst)
+{
+ struct xfrm_dst_list *xl = raw_cpu_ptr(&xfrm_dst_gc_list);
+
+ xdst->xfrm_dst_gc_list = xl;
+
+ spin_lock_bh(&xl->lock);
+ list_add_tail(&xdst->xfrm_dst_gc, &xl->head);
+ spin_unlock_bh(&xl->lock);
+}
+
+void xfrm_dst_destroy(struct xfrm_dst *xdst)
+{
+ xfrm_pols_put(xdst->pols, xdst->num_pols);
+ dst_release(xdst->route);
+ if (likely(xdst->u.dst.xfrm))
+ xfrm_state_put(xdst->u.dst.xfrm);
+ if (!list_empty(&xdst->xfrm_dst_gc)) {
+ struct xfrm_dst_list *xl = xdst->xfrm_dst_gc_list;
+
+ spin_lock_bh(&xl->lock);
+ list_del(&xdst->xfrm_dst_gc);
+ spin_unlock_bh(&xl->lock);
+ }
+}
+EXPORT_SYMBOL_GPL(xfrm_dst_destroy);
+
+static void xfrm_flush_dev(struct net_device *dev)
+{
+ struct xfrm_dst *xdst;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct xfrm_dst_list *xl = &per_cpu(xfrm_dst_gc_list, cpu);
+
+ spin_lock_bh(&xl->lock);
+ list_for_each_entry(xdst, &xl->head, xfrm_dst_gc) {
+ if (xdst->u.dst.dev != dev)
+ continue;
+ dst_dev_put(&xdst->u.dst);
+ }
+ spin_unlock_bh(&xl->lock);
+ }
+}
+
bool xfrm_selector_match(const struct xfrm_selector *sel, const struct flowi *fl,
unsigned short family)
{
@@ -1581,6 +1632,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
}
bundle[i] = xdst;
+ xfrm_dst_add_to_gc_list(xdst);
+
if (!xdst_prev)
xdst0 = xdst;
else
@@ -2937,6 +2990,20 @@ static void xfrm_policy_fini(struct net *net)
xfrm_hash_free(net->xfrm.policy_byidx, sz);
}
+static int xfrm_netdev_event(struct notifier_block *this, unsigned long event,
+ void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+ if (event == NETDEV_UNREGISTER)
+ xfrm_flush_dev(dev);
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block xfrm_netdev_notifier = {
+ .notifier_call = xfrm_netdev_event,
+};
+
static int __net_init xfrm_net_init(struct net *net)
{
int rv;
@@ -2984,6 +3051,19 @@ static struct pernet_operations __net_initdata xfrm_net_ops = {
.exit = xfrm_net_exit,
};
+static void __init xfrm_dst_gc_init(void)
+{
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct xfrm_dst_list *xl = &per_cpu(xfrm_dst_gc_list, cpu);
+
+ INIT_LIST_HEAD(&xl->head);
+ spin_lock_init(&xl->lock);
+ }
+ register_netdevice_notifier(&xfrm_netdev_notifier);
+}
+
void __init xfrm_init(void)
{
int i;
@@ -2998,6 +3078,7 @@ void __init xfrm_init(void)
register_pernet_subsys(&xfrm_net_ops);
seqcount_init(&xfrm_policy_hash_generation);
xfrm_input_init();
+ xfrm_dst_gc_init();
}
#ifdef CONFIG_AUDITSYSCALL
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH ipsec] net: xfrm_policy: fix device unregistration hang
2018-02-12 17:21 [PATCH ipsec] net: xfrm_policy: fix device unregistration hang Eyal Birger
@ 2018-02-12 17:55 ` Shannon Nelson
2018-02-12 18:34 ` Eyal Birger
0 siblings, 1 reply; 3+ messages in thread
From: Shannon Nelson @ 2018-02-12 17:55 UTC (permalink / raw)
To: Eyal Birger, steffen.klassert; +Cc: herbert, davem, netdev, fw, shmulik
On 2/12/2018 9:21 AM, Eyal Birger wrote:
> In setups like the following:
>
> Host A -- Host B
> tun0 -- ipsec -- eth0 -- eth0 -- ipsec -- tun0
>
> where tun0 are tunnel devices using dst_cache (ipip, ipip6, etc...).
>
> Unregistration of an underlying eth0 device leads to the following log
> messages:
>
> unregister_netdevice: waiting for eth0 to become free. Usage count = 2
>
> This is since xfrm dsts device references are not released upon device
> unregistration when the xfrm dst is cached in a dst_cache.
>
> Several approaches for fixing this were discussed in [1]; this commit keeps
> track of allocated xdsts and releases their device references on a netdev
> unregister event.
>
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
>
> [1] https://patchwork.ozlabs.org/patch/869025/
> ---
> include/net/xfrm.h | 10 ++-----
> net/xfrm/xfrm_policy.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 84 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 7d20776..c1c8493 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -994,6 +994,8 @@ struct xfrm_dst {
> u32 child_mtu_cached;
> u32 route_cookie;
> u32 path_cookie;
> + struct list_head xfrm_dst_gc;
> + struct xfrm_dst_list *xfrm_dst_gc_list;
> };
>
> static inline struct dst_entry *xfrm_dst_path(const struct dst_entry *dst)
> @@ -1025,13 +1027,7 @@ static inline void xfrm_dst_set_child(struct xfrm_dst *xdst, struct dst_entry *c
> xdst->child = child;
> }
>
> -static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
> -{
> - xfrm_pols_put(xdst->pols, xdst->num_pols);
> - dst_release(xdst->route);
> - if (likely(xdst->u.dst.xfrm))
> - xfrm_state_put(xdst->u.dst.xfrm);
> -}
> +void xfrm_dst_destroy(struct xfrm_dst *xdst);
> #endif
>
> void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev);
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 7a23078..1da8a65 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -45,6 +45,12 @@ struct xfrm_flo {
> u8 flags;
> };
>
> +struct xfrm_dst_list {
> + spinlock_t lock; /* sync writers */
> + struct list_head head;
> +};
> +
> +static DEFINE_PER_CPU_ALIGNED(struct xfrm_dst_list, xfrm_dst_gc_list);
> static DEFINE_PER_CPU(struct xfrm_dst *, xfrm_last_dst);
> static struct work_struct *xfrm_pcpu_work __read_mostly;
> static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
> @@ -94,6 +100,51 @@ __xfrm6_selector_match(const struct xfrm_selector *sel, const struct flowi *fl)
> (fl6->flowi6_oif == sel->ifindex || !sel->ifindex);
> }
>
> +static void xfrm_dst_add_to_gc_list(struct xfrm_dst *xdst)
> +{
> + struct xfrm_dst_list *xl = raw_cpu_ptr(&xfrm_dst_gc_list);
> +
> + xdst->xfrm_dst_gc_list = xl;
> +
> + spin_lock_bh(&xl->lock);
> + list_add_tail(&xdst->xfrm_dst_gc, &xl->head);
> + spin_unlock_bh(&xl->lock);
> +}
> +
> +void xfrm_dst_destroy(struct xfrm_dst *xdst)
> +{
> + xfrm_pols_put(xdst->pols, xdst->num_pols);
> + dst_release(xdst->route);
> + if (likely(xdst->u.dst.xfrm))
> + xfrm_state_put(xdst->u.dst.xfrm);
> + if (!list_empty(&xdst->xfrm_dst_gc)) {
> + struct xfrm_dst_list *xl = xdst->xfrm_dst_gc_list;
> +
> + spin_lock_bh(&xl->lock);
> + list_del(&xdst->xfrm_dst_gc);
> + spin_unlock_bh(&xl->lock);
> + }
> +}
> +EXPORT_SYMBOL_GPL(xfrm_dst_destroy);
> +
> +static void xfrm_flush_dev(struct net_device *dev)
> +{
> + struct xfrm_dst *xdst;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct xfrm_dst_list *xl = &per_cpu(xfrm_dst_gc_list, cpu);
> +
> + spin_lock_bh(&xl->lock);
> + list_for_each_entry(xdst, &xl->head, xfrm_dst_gc) {
> + if (xdst->u.dst.dev != dev)
> + continue;
> + dst_dev_put(&xdst->u.dst);
> + }
> + spin_unlock_bh(&xl->lock);
> + }
> +}
> +
> bool xfrm_selector_match(const struct xfrm_selector *sel, const struct flowi *fl,
> unsigned short family)
> {
> @@ -1581,6 +1632,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> }
>
> bundle[i] = xdst;
> + xfrm_dst_add_to_gc_list(xdst);
> +
> if (!xdst_prev)
> xdst0 = xdst;
> else
> @@ -2937,6 +2990,20 @@ static void xfrm_policy_fini(struct net *net)
> xfrm_hash_free(net->xfrm.policy_byidx, sz);
> }
>
> +static int xfrm_netdev_event(struct notifier_block *this, unsigned long event,
> + void *ptr)
> +{
> + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +
> + if (event == NETDEV_UNREGISTER)
> + xfrm_flush_dev(dev);
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block xfrm_netdev_notifier = {
> + .notifier_call = xfrm_netdev_event,
> +};
> +
Instead of creating yet another netdev notifier, can this be wired into
xfrm_dev_unregister(), the existing notifier in xfrm_decide.c which
calls into xfrm_policy_cache_flush()?
sln
> static int __net_init xfrm_net_init(struct net *net)
> {
> int rv;
> @@ -2984,6 +3051,19 @@ static struct pernet_operations __net_initdata xfrm_net_ops = {
> .exit = xfrm_net_exit,
> };
>
> +static void __init xfrm_dst_gc_init(void)
> +{
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct xfrm_dst_list *xl = &per_cpu(xfrm_dst_gc_list, cpu);
> +
> + INIT_LIST_HEAD(&xl->head);
> + spin_lock_init(&xl->lock);
> + }
> + register_netdevice_notifier(&xfrm_netdev_notifier);
> +}
> +
> void __init xfrm_init(void)
> {
> int i;
> @@ -2998,6 +3078,7 @@ void __init xfrm_init(void)
> register_pernet_subsys(&xfrm_net_ops);
> seqcount_init(&xfrm_policy_hash_generation);
> xfrm_input_init();
> + xfrm_dst_gc_init();
> }
>
> #ifdef CONFIG_AUDITSYSCALL
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH ipsec] net: xfrm_policy: fix device unregistration hang
2018-02-12 17:55 ` Shannon Nelson
@ 2018-02-12 18:34 ` Eyal Birger
0 siblings, 0 replies; 3+ messages in thread
From: Eyal Birger @ 2018-02-12 18:34 UTC (permalink / raw)
To: Shannon Nelson; +Cc: steffen.klassert, herbert, davem, netdev, fw, shmulik
On Mon, 12 Feb 2018 09:55:48 -0800
Shannon Nelson <shannon.nelson@oracle.com> wrote:
> On 2/12/2018 9:21 AM, Eyal Birger wrote:
> > In setups like the following:
> >
> > Host A -- Host B
> > tun0 -- ipsec -- eth0 -- eth0 -- ipsec -- tun0
> >
> > where tun0 are tunnel devices using dst_cache (ipip, ipip6, etc...).
> >
> > Unregistration of an underlying eth0 device leads to the following
> > log messages:
> >
> > unregister_netdevice: waiting for eth0 to become free. Usage count
> > = 2
> >
> > This is since xfrm dsts device references are not released upon
> > device unregistration when the xfrm dst is cached in a dst_cache.
> >
> > Several approaches for fixing this were discussed in [1]; this
> > commit keeps track of allocated xdsts and releases their device
> > references on a netdev unregister event.
> >
> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> >
> > [1] https://patchwork.ozlabs.org/patch/869025/
> > ---
> > include/net/xfrm.h | 10 ++-----
> > net/xfrm/xfrm_policy.c | 81
> > ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed,
> > 84 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index 7d20776..c1c8493 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -994,6 +994,8 @@ struct xfrm_dst {
> > u32 child_mtu_cached;
> > u32 route_cookie;
> > u32 path_cookie;
> > + struct list_head xfrm_dst_gc;
> > + struct xfrm_dst_list *xfrm_dst_gc_list;
> > };
> >
> > static inline struct dst_entry *xfrm_dst_path(const struct
> > dst_entry *dst) @@ -1025,13 +1027,7 @@ static inline void
> > xfrm_dst_set_child(struct xfrm_dst *xdst, struct dst_entry *c
> > xdst->child = child; }
> >
> > -static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
> > -{
> > - xfrm_pols_put(xdst->pols, xdst->num_pols);
> > - dst_release(xdst->route);
> > - if (likely(xdst->u.dst.xfrm))
> > - xfrm_state_put(xdst->u.dst.xfrm);
> > -}
> > +void xfrm_dst_destroy(struct xfrm_dst *xdst);
> > #endif
> >
> > void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device
> > *dev); diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 7a23078..1da8a65 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -45,6 +45,12 @@ struct xfrm_flo {
> > u8 flags;
> > };
> >
> > +struct xfrm_dst_list {
> > + spinlock_t lock; /* sync writers */
> > + struct list_head head;
> > +};
> > +
> > +static DEFINE_PER_CPU_ALIGNED(struct xfrm_dst_list,
> > xfrm_dst_gc_list); static DEFINE_PER_CPU(struct xfrm_dst *,
> > xfrm_last_dst); static struct work_struct *xfrm_pcpu_work
> > __read_mostly; static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
> > @@ -94,6 +100,51 @@ __xfrm6_selector_match(const struct
> > xfrm_selector *sel, const struct flowi *fl) (fl6->flowi6_oif ==
> > sel->ifindex || !sel->ifindex); }
> >
> > +static void xfrm_dst_add_to_gc_list(struct xfrm_dst *xdst)
> > +{
> > + struct xfrm_dst_list *xl = raw_cpu_ptr(&xfrm_dst_gc_list);
> > +
> > + xdst->xfrm_dst_gc_list = xl;
> > +
> > + spin_lock_bh(&xl->lock);
> > + list_add_tail(&xdst->xfrm_dst_gc, &xl->head);
> > + spin_unlock_bh(&xl->lock);
> > +}
> > +
> > +void xfrm_dst_destroy(struct xfrm_dst *xdst)
> > +{
> > + xfrm_pols_put(xdst->pols, xdst->num_pols);
> > + dst_release(xdst->route);
> > + if (likely(xdst->u.dst.xfrm))
> > + xfrm_state_put(xdst->u.dst.xfrm);
> > + if (!list_empty(&xdst->xfrm_dst_gc)) {
> > + struct xfrm_dst_list *xl = xdst->xfrm_dst_gc_list;
> > +
> > + spin_lock_bh(&xl->lock);
> > + list_del(&xdst->xfrm_dst_gc);
> > + spin_unlock_bh(&xl->lock);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(xfrm_dst_destroy);
> > +
> > +static void xfrm_flush_dev(struct net_device *dev)
> > +{
> > + struct xfrm_dst *xdst;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + struct xfrm_dst_list *xl =
> > &per_cpu(xfrm_dst_gc_list, cpu); +
> > + spin_lock_bh(&xl->lock);
> > + list_for_each_entry(xdst, &xl->head, xfrm_dst_gc) {
> > + if (xdst->u.dst.dev != dev)
> > + continue;
> > + dst_dev_put(&xdst->u.dst);
> > + }
> > + spin_unlock_bh(&xl->lock);
> > + }
> > +}
> > +
> > bool xfrm_selector_match(const struct xfrm_selector *sel, const
> > struct flowi *fl, unsigned short family)
> > {
> > @@ -1581,6 +1632,8 @@ static struct dst_entry
> > *xfrm_bundle_create(struct xfrm_policy *policy, }
> >
> > bundle[i] = xdst;
> > + xfrm_dst_add_to_gc_list(xdst);
> > +
> > if (!xdst_prev)
> > xdst0 = xdst;
> > else
> > @@ -2937,6 +2990,20 @@ static void xfrm_policy_fini(struct net *net)
> > xfrm_hash_free(net->xfrm.policy_byidx, sz);
> > }
> >
> > +static int xfrm_netdev_event(struct notifier_block *this, unsigned
> > long event,
> > + void *ptr)
> > +{
> > + struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > +
> > + if (event == NETDEV_UNREGISTER)
> > + xfrm_flush_dev(dev);
> > + return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block xfrm_netdev_notifier = {
> > + .notifier_call = xfrm_netdev_event,
> > +};
> > +
>
> Instead of creating yet another netdev notifier, can this be wired
> into xfrm_dev_unregister(), the existing notifier in xfrm_decide.c
> which calls into xfrm_policy_cache_flush()?
I don't mind doing so... I was under the impression that xfrm_device.c
is designated to device offload (as indicated by the top level comment) -
though as you mentioned, xfrm_policy_cache_flush() is indeed called from
there.
Thanks!
Eyal.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-12 18:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 17:21 [PATCH ipsec] net: xfrm_policy: fix device unregistration hang Eyal Birger
2018-02-12 17:55 ` Shannon Nelson
2018-02-12 18:34 ` Eyal Birger
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.