Skip to contentSkip to conversation

vxlan: unregister on namespace exit

PATCH Applied Stephen Hemminger 3 participants 7 messages Jul 18, 2013

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/2 vxlan: unregister on namespace exit

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 <[email protected]>
drivers/net/vxlan.c +4 -2
@@ -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();
 }
author
#1 lore raw

PATCH 2/2 vxlan: fix igmp races

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 <[email protected]>
drivers/net/vxlan.c +38 -15
@@ -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);
maintainer Applied
#2 lore raw

Re: [PATCH net 2/2] vxlan: fix igmp races

From: Stephen Hemminger <[email protected]>
Date: Thu, 18 Jul 2013 08:40:15 -0700

Quoted 11 lines

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 <[email protected]>

Applied, thanks Stephen.

Reviewed-by on v1 1/2
#3 lore raw
In reply to Stephen Hemminger
Quoted 14 lines · Stephen Hemminger

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 <[email protected]>
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +1 -0
@@ -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);

drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 static __net_exit void vxlan_exit_net(st · line 1889

Quoted 10 lines
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +3 -2
 
        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 <[email protected]>
author
#4 lore raw
Quoted 27 lines · On Thu, 18 Jul 2013 09:40:07 -0700 Pravin Shelar <[email protected]> wrote:
Quoted 14 lines · Stephen Hemminger

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 <[email protected]>
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +1 -0
@@ -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);

drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 static __net_exit void vxlan_exit_net(st · line 1889

Quoted 10 lines
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +3 -2
 
        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.

Quoted 1 line
Reviewed-by Pravin B Shelar <[email protected]>

vxlan might be a candidate for integration with the generic ip_tunnel code, but haven't looked at all the details.

#5 lore raw
Quoted 32 lines · Stephen Hemminger
Quoted 27 lines · On Thu, 18 Jul 2013 09:40:07 -0700 Pravin Shelar <[email protected]> wrote:
Quoted 14 lines · Stephen Hemminger

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 <[email protected]>
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +1 -0
@@ -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);

drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 static __net_exit void vxlan_exit_net(st · line 1889

Quoted 10 lines
drivers/net/vxlan.c 2013-07-17 16:47:35.275690741 -0700 renamed from drivers/net/vxlan.c 2013-07-17 16:32:10.826955800 -0700 +3 -2
 
        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.

Quoted 1 line
Reviewed-by Pravin B Shelar <[email protected]>
Quoted 2 lines

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.