* [PATCH net 1/2] vxlan: unregister on namespace exit
@ 2013-07-18 15:38 Stephen Hemminger
2013-07-18 15:40 ` [PATCH net 2/2] vxlan: fix igmp races Stephen Hemminger
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Stephen Hemminger @ 2013-07-18 15:38 UTC (permalink / raw)
To: David Miller, Pravin Shelar; +Cc: netdev
Fix memory leaks and other badness from VXLAN network namespace
teardown. When network namespace is removed, all the vxlan devices should
be unregistered (not closed).
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
--- a/drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700
+++ b/drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700
@@ -1878,10 +1878,12 @@ static __net_exit void vxlan_exit_net(st
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_dev *vxlan;
+ LIST_HEAD(list);
rtnl_lock();
- list_for_each_entry(vxlan, &vn->vxlan_list, next)
- dev_close(vxlan->dev);
+ list_for_each_entry(vxlan, &vn->vxlan_list, next)
+ unregister_netdevice_queue(vxlan->dev, &list);
+ unregister_netdevice_many(&list);
rtnl_unlock();
}
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net 2/2] vxlan: fix igmp races 2013-07-18 15:38 [PATCH net 1/2] vxlan: unregister on namespace exit Stephen Hemminger @ 2013-07-18 15:40 ` Stephen Hemminger 2013-07-18 20:15 ` David Miller 2013-07-18 16:40 ` [PATCH net 1/2] vxlan: unregister on namespace exit Pravin Shelar 2013-07-18 20:15 ` David Miller 2 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2013-07-18 15:40 UTC (permalink / raw) To: David Miller, Pravin Shelar; +Cc: netdev There are two race conditions in existing code for doing IGMP management in workqueue in vxlan. First, the vxlan_group_used function checks the list of vxlan's without any protection, and it is possible for open followed by close to occur before the igmp work queue runs. To solve these move the check into vxlan_open/stop so it is protected by RTNL. And split into two work structures so that there is no racy reference to underlying device state. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- a/drivers/net/vxlan.c 2013-07-17 17:03:05.136038741 -0700 +++ b/drivers/net/vxlan.c 2013-07-17 17:31:33.822426541 -0700 @@ -136,7 +136,8 @@ struct vxlan_dev { u32 flags; /* VXLAN_F_* below */ struct work_struct sock_work; - struct work_struct igmp_work; + struct work_struct igmp_join; + struct work_struct igmp_leave; unsigned long age_interval; struct timer_list age_timer; @@ -736,7 +737,6 @@ static bool vxlan_snoop(struct net_devic return false; } - /* See if multicast group is already in use by other ID */ static bool vxlan_group_used(struct vxlan_net *vn, __be32 remote_ip) { @@ -770,12 +770,13 @@ static void vxlan_sock_release(struct vx queue_work(vxlan_wq, &vs->del_work); } -/* Callback to update multicast group membership. - * Scheduled when vxlan goes up/down. +/* Callback to update multicast group membership when first VNI on + * multicast asddress is brought up + * Done as workqueue because ip_mc_join_group acquires RTNL. */ -static void vxlan_igmp_work(struct work_struct *work) +static void vxlan_igmp_join(struct work_struct *work) { - struct vxlan_dev *vxlan = container_of(work, struct vxlan_dev, igmp_work); + struct vxlan_dev *vxlan = container_of(work, struct vxlan_dev, igmp_join); struct vxlan_net *vn = net_generic(dev_net(vxlan->dev), vxlan_net_id); struct vxlan_sock *vs = vxlan->vn_sock; struct sock *sk = vs->sock->sk; @@ -785,10 +786,27 @@ static void vxlan_igmp_work(struct work_ }; lock_sock(sk); - if (vxlan_group_used(vn, vxlan->default_dst.remote_ip)) - ip_mc_join_group(sk, &mreq); - else - ip_mc_leave_group(sk, &mreq); + ip_mc_join_group(sk, &mreq); + release_sock(sk); + + vxlan_sock_release(vn, vs); + dev_put(vxlan->dev); +} + +/* Inverse of vxlan_igmp_join when last VNI is brought down */ +static void vxlan_igmp_leave(struct work_struct *work) +{ + struct vxlan_dev *vxlan = container_of(work, struct vxlan_dev, igmp_leave); + struct vxlan_net *vn = net_generic(dev_net(vxlan->dev), vxlan_net_id); + struct vxlan_sock *vs = vxlan->vn_sock; + struct sock *sk = vs->sock->sk; + struct ip_mreqn mreq = { + .imr_multiaddr.s_addr = vxlan->default_dst.remote_ip, + .imr_ifindex = vxlan->default_dst.remote_ifindex, + }; + + lock_sock(sk); + ip_mc_leave_group(sk, &mreq); release_sock(sk); vxlan_sock_release(vn, vs); @@ -1359,6 +1377,7 @@ static void vxlan_uninit(struct net_devi /* Start ageing timer and join group when device is brought up */ static int vxlan_open(struct net_device *dev) { + struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id); struct vxlan_dev *vxlan = netdev_priv(dev); struct vxlan_sock *vs = vxlan->vn_sock; @@ -1366,10 +1385,11 @@ static int vxlan_open(struct net_device if (!vs) return -ENOTCONN; - if (IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip))) { + if (IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip)) && + ! vxlan_group_used(vn, vxlan->default_dst.remote_ip)) { vxlan_sock_hold(vs); dev_hold(dev); - queue_work(vxlan_wq, &vxlan->igmp_work); + queue_work(vxlan_wq, &vxlan->igmp_join); } if (vxlan->age_interval) @@ -1400,13 +1420,15 @@ static void vxlan_flush(struct vxlan_dev /* Cleanup timer and forwarding table on shutdown */ static int vxlan_stop(struct net_device *dev) { + struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id); struct vxlan_dev *vxlan = netdev_priv(dev); struct vxlan_sock *vs = vxlan->vn_sock; - if (vs && IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip))) { + if (vs && IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip)) && + ! vxlan_group_used(vn, vxlan->default_dst.remote_ip)) { vxlan_sock_hold(vs); dev_hold(dev); - queue_work(vxlan_wq, &vxlan->igmp_work); + queue_work(vxlan_wq, &vxlan->igmp_leave); } del_timer_sync(&vxlan->age_timer); @@ -1471,7 +1493,8 @@ static void vxlan_setup(struct net_devic INIT_LIST_HEAD(&vxlan->next); spin_lock_init(&vxlan->hash_lock); - INIT_WORK(&vxlan->igmp_work, vxlan_igmp_work); + INIT_WORK(&vxlan->igmp_join, vxlan_igmp_join); + INIT_WORK(&vxlan->igmp_leave, vxlan_igmp_leave); INIT_WORK(&vxlan->sock_work, vxlan_sock_work); init_timer_deferrable(&vxlan->age_timer); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] vxlan: fix igmp races 2013-07-18 15:40 ` [PATCH net 2/2] vxlan: fix igmp races Stephen Hemminger @ 2013-07-18 20:15 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2013-07-18 20:15 UTC (permalink / raw) To: stephen; +Cc: pshelar, netdev From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 18 Jul 2013 08:40:15 -0700 > There are two race conditions in existing code for doing IGMP > management in workqueue in vxlan. First, the vxlan_group_used > function checks the list of vxlan's without any protection, and > it is possible for open followed by close to occur before the > igmp work queue runs. > > To solve these move the check into vxlan_open/stop so it is > protected by RTNL. And split into two work structures so that > there is no racy reference to underlying device state. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Applied, thanks Stephen. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] vxlan: unregister on namespace exit 2013-07-18 15:38 [PATCH net 1/2] vxlan: unregister on namespace exit Stephen Hemminger 2013-07-18 15:40 ` [PATCH net 2/2] vxlan: fix igmp races Stephen Hemminger @ 2013-07-18 16:40 ` Pravin Shelar 2013-07-18 16:58 ` Stephen Hemminger 2013-07-18 20:15 ` David Miller 2 siblings, 1 reply; 7+ messages in thread From: Pravin Shelar @ 2013-07-18 16:40 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, netdev On Thu, Jul 18, 2013 at 8:38 AM, Stephen Hemminger <stephen@networkplumber.org> wrote: > Fix memory leaks and other badness from VXLAN network namespace > teardown. When network namespace is removed, all the vxlan devices should > be unregistered (not closed). > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > > --- a/drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 > +++ b/drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 > @@ -1878,10 +1878,12 @@ static __net_exit void vxlan_exit_net(st > { > struct vxlan_net *vn = net_generic(net, vxlan_net_id); > struct vxlan_dev *vxlan; > + LIST_HEAD(list); > > rtnl_lock(); > - list_for_each_entry(vxlan, &vn->vxlan_list, next) > - dev_close(vxlan->dev); > + list_for_each_entry(vxlan, &vn->vxlan_list, next) > + unregister_netdevice_queue(vxlan->dev, &list); > + unregister_netdevice_many(&list); > rtnl_unlock(); > } > Looks good. Reviewed-by: Pravin B Shelar <pshelar@nicira.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] vxlan: unregister on namespace exit 2013-07-18 16:40 ` [PATCH net 1/2] vxlan: unregister on namespace exit Pravin Shelar @ 2013-07-18 16:58 ` Stephen Hemminger 2013-07-18 18:28 ` Pravin Shelar 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2013-07-18 16:58 UTC (permalink / raw) To: Pravin Shelar; +Cc: David Miller, netdev On Thu, 18 Jul 2013 09:40:07 -0700 Pravin Shelar <pshelar@nicira.com> wrote: > On Thu, Jul 18, 2013 at 8:38 AM, Stephen Hemminger > <stephen@networkplumber.org> wrote: > > Fix memory leaks and other badness from VXLAN network namespace > > teardown. When network namespace is removed, all the vxlan devices should > > be unregistered (not closed). > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > > > > > --- a/drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 > > +++ b/drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 > > @@ -1878,10 +1878,12 @@ static __net_exit void vxlan_exit_net(st > > { > > struct vxlan_net *vn = net_generic(net, vxlan_net_id); > > struct vxlan_dev *vxlan; > > + LIST_HEAD(list); > > > > rtnl_lock(); > > - list_for_each_entry(vxlan, &vn->vxlan_list, next) > > - dev_close(vxlan->dev); > > + list_for_each_entry(vxlan, &vn->vxlan_list, next) > > + unregister_netdevice_queue(vxlan->dev, &list); > > + unregister_netdevice_many(&list); > > rtnl_unlock(); > > } > > > Looks good. > Reviewed-by: Pravin B Shelar <pshelar@nicira.com> vxlan might be a candidate for integration with the generic ip_tunnel code, but haven't looked at all the details. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] vxlan: unregister on namespace exit 2013-07-18 16:58 ` Stephen Hemminger @ 2013-07-18 18:28 ` Pravin Shelar 0 siblings, 0 replies; 7+ messages in thread From: Pravin Shelar @ 2013-07-18 18:28 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, netdev On Thu, Jul 18, 2013 at 9:58 AM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Thu, 18 Jul 2013 09:40:07 -0700 > Pravin Shelar <pshelar@nicira.com> wrote: > >> On Thu, Jul 18, 2013 at 8:38 AM, Stephen Hemminger >> <stephen@networkplumber.org> wrote: >> > Fix memory leaks and other badness from VXLAN network namespace >> > teardown. When network namespace is removed, all the vxlan devices should >> > be unregistered (not closed). >> > >> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >> > >> > >> > --- a/drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 >> > +++ b/drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 >> > @@ -1878,10 +1878,12 @@ static __net_exit void vxlan_exit_net(st >> > { >> > struct vxlan_net *vn = net_generic(net, vxlan_net_id); >> > struct vxlan_dev *vxlan; >> > + LIST_HEAD(list); >> > >> > rtnl_lock(); >> > - list_for_each_entry(vxlan, &vn->vxlan_list, next) >> > - dev_close(vxlan->dev); >> > + list_for_each_entry(vxlan, &vn->vxlan_list, next) >> > + unregister_netdevice_queue(vxlan->dev, &list); >> > + unregister_netdevice_many(&list); >> > rtnl_unlock(); >> > } >> > >> Looks good. >> Reviewed-by: Pravin B Shelar <pshelar@nicira.com> > > vxlan might be a candidate for integration with the generic ip_tunnel > code, but haven't looked at all the details. ip_tunnel already does same on net-exit. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] vxlan: unregister on namespace exit 2013-07-18 15:38 [PATCH net 1/2] vxlan: unregister on namespace exit Stephen Hemminger 2013-07-18 15:40 ` [PATCH net 2/2] vxlan: fix igmp races Stephen Hemminger 2013-07-18 16:40 ` [PATCH net 1/2] vxlan: unregister on namespace exit Pravin Shelar @ 2013-07-18 20:15 ` David Miller 2 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2013-07-18 20:15 UTC (permalink / raw) To: stephen; +Cc: pshelar, netdev From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 18 Jul 2013 08:38:26 -0700 > Fix memory leaks and other badness from VXLAN network namespace > teardown. When network namespace is removed, all the vxlan devices should > be unregistered (not closed). > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Applied. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-07-18 20:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-18 15:38 [PATCH net 1/2] vxlan: unregister on namespace exit Stephen Hemminger 2013-07-18 15:40 ` [PATCH net 2/2] vxlan: fix igmp races Stephen Hemminger 2013-07-18 20:15 ` David Miller 2013-07-18 16:40 ` [PATCH net 1/2] vxlan: unregister on namespace exit Pravin Shelar 2013-07-18 16:58 ` Stephen Hemminger 2013-07-18 18:28 ` Pravin Shelar 2013-07-18 20:15 ` David Miller
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).