Skip to contentSkip to conversation

vxlan: only migrate dynamic FDB entries

PATCH Superseded Stephen Hemminger 6 participants 30 messages Jun 5, 2013 – Jun 8, 2013

Subsystems (2)
Networking Network drivers

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/10 vxlan: only migrate dynamic FDB entries

Only migrate dynamic forwarding table entries, don't modify static entries. If packet received from incorrect source IP address assume it is an imposter and drop it.

Signed-off-by Stephen Hemminger <[email protected]>

---
Should go to -stable as well.

---
 drivers/net/vxlan.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/net/vxlan.c +12 -5
index 8111565..536082a 100644
@@ -604,8 +604,8 @@ skip:
 /* Watch incoming packets to learn mapping between Ethernet address
  * and Tunnel endpoint.
  */
-static void vxlan_snoop(struct net_device *dev,
-			__be32 src_ip, const u8 *src_mac)
+static int vxlan_snoop(struct net_device *dev,
+		       __be32 src_ip, const u8 *src_mac)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_fdb *f;

@@ -614,7 +614,11 @@ static void vxlan_snoop(struct net_device *dev,
 	f = vxlan_find_mac(vxlan, src_mac);
 	if (likely(f)) {
 		if (likely(f->remote.remote_ip == src_ip))
-			return;
+			return 0;
+
+		/* Don't migrate static entries, drop packets */
+		if (!(f->flags & NTF_SELF))
+			return 1;
 
 		if (net_ratelimit())
 			netdev_info(dev,

@@ -634,6 +638,8 @@ static void vxlan_snoop(struct net_device *dev,
 				       0, NTF_SELF);
 		spin_unlock(&vxlan->hash_lock);
 	}
+
+	return 0;
 }
 
 

@@ -766,8 +772,9 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 			       vxlan->dev->dev_addr) == 0)
 		goto drop;
 
-	if (vxlan->flags & VXLAN_F_LEARN)
-		vxlan_snoop(skb->dev, oip->saddr, eth_hdr(skb)->h_source);
+	if ((vxlan->flags & VXLAN_F_LEARN) &&
+	    vxlan_snoop(skb->dev, oip->saddr, eth_hdr(skb)->h_source))
+		goto drop;
 
 	__skb_tunnel_rx(skb, vxlan->dev);
 	skb_reset_network_header(skb);
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 2/10 vxlan: handle skb_clone failure

skb_clone can fail if out of memory. Just skip the fanout.

Signed-off-by Stephen Hemminger <[email protected]>

---
Does not impact stable, this was introduced by the multiple destination code.

---
 drivers/net/vxlan.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/vxlan.c +5 -3
index 536082a..9085c81 100644
@@ -1197,9 +1197,11 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 		struct sk_buff *skb1;
 
 		skb1 = skb_clone(skb, GFP_ATOMIC);
-		rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-		if (rc == NETDEV_TX_OK)
-			rc = rc1;
+		if (skb1) {
+			rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+			if (rc == NETDEV_TX_OK)
+				rc = rc1;
+		}
 	}
 
 	rc1 = vxlan_xmit_one(skb, dev, rdst0, did_rsc);
Signature · 1 line
1.7.10.4
#2 lore raw

Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure

drivers/net/vxlan.c static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev) · line 1204

Quoted 9 lines · Stephen Hemminger
drivers/net/vxlan.c +5 -3
 		skb1 = skb_clone(skb, GFP_ATOMIC);
-		rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-		if (rc == NETDEV_TX_OK)
-			rc = rc1;
+		if (skb1) {
+			rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+			if (rc == NETDEV_TX_OK)
+				rc = rc1;
+		}

If OOM, shouldn't we exit immediately instead of continue handle the next one?

#3 lore raw

Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure

[email protected] wrote on 06/05/2013 02:59:56 AM:

Quoted 18 lines

From: Cong Wang <[email protected]>
To: [email protected]
Date: 06/05/2013 03:00 AM
Subject: Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure Sent by: [email protected]

drivers/net/vxlan.c static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev) · line 1204

Quoted 9 lines · Stephen Hemminger
       skb1 = skb_clone(skb, GFP_ATOMIC);
drivers/net/vxlan.c +5 -3
-      rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-      if (rc == NETDEV_TX_OK)
-         rc = rc1;
+      if (skb1) {
+         rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+         if (rc == NETDEV_TX_OK)
+            rc = rc1;
+      }
Quoted 2 lines

If OOM, shouldn't we exit immediately instead of continue handle the next one?

It could be a temporary condition; trying again doesn't really hurt, in case some do go through. So, I prefer this, but could live with bailing for all destinations, too.

                                                        +-DLS
#4 lore raw

Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure

Quoted 10 lines · David Stevens

[email protected] wrote on 06/05/2013 02:59:56 AM:

Quoted 3 lines

From: Cong Wang <[email protected]>
If OOM, shouldn't we exit immediately instead of continue handle the next one?

It could be a temporary condition; trying again doesn't really hurt, in case some do go through. So, I prefer this, but could live with bailing for all destinations, too.

The problem we don't know if it is temporary or not, unless mm provides EAGAIN?

author
#5 lore raw

Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure

Quoted 12 lines · On Thu, 6 Jun 2013 08:47:49 +0800 Cong Wang <[email protected]> wrote:
Quoted 10 lines · David Stevens

[email protected] wrote on 06/05/2013 02:59:56 AM:

Quoted 3 lines

From: Cong Wang <[email protected]>
If OOM, shouldn't we exit immediately instead of continue handle the next one?

It could be a temporary condition; trying again doesn't really hurt, in case some do go through. So, I prefer this, but could live with bailing for all destinations, too.

Quoted 6 lines

The problem we don't know if it is temporary or not, unless mm provides EAGAIN?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

There is no EAGAIN in the network transmit path, it is not called from user context.

Acked-by on v1 2/10
#6 lore raw

Re: [PATCH net-next 02/10] vxlan: handle skb_clone failure

In reply to Stephen Hemminger
Acked-by David L Stevens <[email protected]>
Quoted 41 lines

From: Stephen Hemminger <[email protected]>

skb_clone can fail if out of memory. Just skip the fanout.

Signed-off-by Stephen Hemminger <[email protected]>

---
Does not impact stable, this was introduced by the multiple destination code.

---
 drivers/net/vxlan.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/vxlan.c +5 -3
index 536082a..9085c81 100644
@@ -1197,9 +1197,11 @@ static netdev_tx_t vxlan_xmit(struct sk_buff 
 *skb, struct net_device *dev)
       struct sk_buff *skb1;
 
       skb1 = skb_clone(skb, GFP_ATOMIC);
-      rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-      if (rc == NETDEV_TX_OK)
-         rc = rc1;
+      if (skb1) {
+         rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+         if (rc == NETDEV_TX_OK)
+            rc = rc1;
+      }
    }
 

    rc1 = vxlan_xmit_one(skb, dev, rdst0, did_rsc);
Signature · 6 lines
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#7 lore raw

PATCH 3/10 vxlan: move IGMP join/leave to work queue

In reply to Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

Signed-off-by Stephen Hemminger <[email protected]>

---
The last change probably needs a separate patch for -stable

---
 drivers/net/vxlan.c |  105 ++++++++++++++++++++-------------------------------
 1 file changed, 41 insertions(+), 64 deletions(-)
drivers/net/vxlan.c +41 -64
index 9085c81..f98f459 100644
@@ -85,7 +85,7 @@ struct vxlan_sock {
 	struct hlist_node hlist;
 	struct rcu_head	  rcu;
 	struct work_struct del_work;
-	unsigned int	  refcnt;
+	atomic_t	  refcnt;
 	struct socket	  *sock;
 	struct hlist_head vni_list[VNI_HASH_SIZE];
 };

@@ -131,6 +131,7 @@ struct vxlan_dev {
 	__u8		  ttl;
 	u32		  flags;	/* VXLAN_F_* below */
 
+	struct work_struct igmp_work;
 	unsigned long	  age_interval;
 	struct timer_list age_timer;
 	spinlock_t	  hash_lock;

@@ -644,76 +645,58 @@ static int vxlan_snoop(struct net_device *dev,
 
 
 /* See if multicast group is already in use by other ID */
-static bool vxlan_group_used(struct vxlan_net *vn,
-			     const struct vxlan_dev *this)
+static bool vxlan_group_used(struct vxlan_net *vn, __be32 remote_ip)
 {
 	struct vxlan_dev *vxlan;
 
 	list_for_each_entry(vxlan, &vn->vxlan_list, next) {
-		if (vxlan == this)
-			continue;
-
 		if (!netif_running(vxlan->dev))
 			continue;
 
-		if (vxlan->default_dst.remote_ip == this->default_dst.remote_ip)
+		if (vxlan->default_dst.remote_ip == remote_ip)
 			return true;
 	}
 
 	return false;
 }
 
-/* kernel equivalent to IP_ADD_MEMBERSHIP */
-static int vxlan_join_group(struct net_device *dev)
+static void vxlan_sock_hold(struct vxlan_sock *vs)
 {
-	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
-	struct sock *sk = vxlan->vn_sock->sock->sk;
-	struct ip_mreqn mreq = {
-		.imr_multiaddr.s_addr	= vxlan->default_dst.remote_ip,
-		.imr_ifindex		= vxlan->default_dst.remote_ifindex,
-	};
-	int err;
-
-	/* Already a member of group */
-	if (vxlan_group_used(vn, vxlan))
-		return 0;
+	atomic_inc(&vs->refcnt);
+}
 
-	/* Need to drop RTNL to call multicast join */
-	rtnl_unlock();
-	lock_sock(sk);
-	err = ip_mc_join_group(sk, &mreq);
-	release_sock(sk);
-	rtnl_lock();
+static void vxlan_sock_release(struct vxlan_sock *vs)
+{
+	if (!atomic_dec_and_test(&vs->refcnt))
+		return;
 
-	return err;
+	hlist_del_rcu(&vs->hlist);
+	schedule_work(&vs->del_work);
 }
 
-
-/* kernel equivalent to IP_DROP_MEMBERSHIP */
-static int vxlan_leave_group(struct net_device *dev)
+/* Callback to update multicast group membership.
+ * Scheduled when vxlan goes up/down.
+ */
+static void vxlan_igmp_work(struct work_struct *work)
 {
-	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
-	int err = 0;
-	struct sock *sk = vxlan->vn_sock->sock->sk;
+	struct vxlan_dev *vxlan = container_of(work, struct vxlan_dev, igmp_work);
+	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,
 	};
 
-	/* Only leave group when last vxlan is done. */
-	if (vxlan_group_used(vn, vxlan))
-		return 0;
-
-	/* Need to drop RTNL to call multicast leave */
-	rtnl_unlock();
 	lock_sock(sk);
-	err = ip_mc_leave_group(sk, &mreq);
+	if (vxlan_group_used(vn, vxlan->default_dst.remote_ip))
+		ip_mc_join_group(sk, &mreq);
+	else
+		ip_mc_leave_group(sk, &mreq);
 	release_sock(sk);
-	rtnl_lock();
 
-	return err;
+	vxlan_sock_release(vs);
+	dev_put(vxlan->dev);
 }
 
 /* Callback from net/ipv4/udp.c to receive packets */

@@ -1261,12 +1244,11 @@ static int vxlan_init(struct net_device *dev)
 static int vxlan_open(struct net_device *dev)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
-	int err;
 
 	if (IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip))) {
-		err = vxlan_join_group(dev);
-		if (err)
-			return err;
+		vxlan_sock_hold(vxlan->vn_sock);
+		dev_hold(dev);
+		schedule_work(&vxlan->igmp_work);
 	}
 
 	if (vxlan->age_interval)

@@ -1297,8 +1279,11 @@ static int vxlan_stop(struct net_device *dev)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 
-	if (IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip)))
-		vxlan_leave_group(dev);
+	if (IN_MULTICAST(ntohl(vxlan->default_dst.remote_ip))) {
+		vxlan_sock_hold(vxlan->vn_sock);
+		dev_hold(dev);
+		schedule_work(&vxlan->igmp_work);
+	}
 
 	del_timer_sync(&vxlan->age_timer);
 

@@ -1367,6 +1352,7 @@ static void vxlan_setup(struct net_device *dev)
 
 	INIT_LIST_HEAD(&vxlan->next);
 	spin_lock_init(&vxlan->hash_lock);
+	INIT_WORK(&vxlan->igmp_work, vxlan_igmp_work);
 
 	init_timer_deferrable(&vxlan->age_timer);
 	vxlan->age_timer.function = vxlan_cleanup;

@@ -1510,8 +1496,8 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
 	udp_sk(sk)->encap_type = 1;
 	udp_sk(sk)->encap_rcv = vxlan_udp_encap_recv;
 	udp_encap_enable();
+	atomic_set(&vs->refcnt, 1);
 
-	vs->refcnt = 1;
 	return vs;
 }
 

@@ -1601,7 +1587,7 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 
 	vs = vxlan_find_port(net, vxlan->dst_port);
 	if (vs)
-		++vs->refcnt;
+		atomic_inc(&vs->refcnt);
 	else {
 		/* Drop lock because socket create acquires RTNL lock */
 		rtnl_unlock();

@@ -1618,12 +1604,7 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
 
 	err = register_netdevice(dev);
 	if (err) {
-		if (--vs->refcnt == 0) {
-			rtnl_unlock();
-			sk_release_kernel(vs->sock->sk);
-			kfree(vs);
-			rtnl_lock();
-		}
+		vxlan_sock_release(vs);
 		return err;
 	}
 

@@ -1641,11 +1622,7 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
 	hlist_del_rcu(&vxlan->hlist);
 	list_del(&vxlan->next);
 	unregister_netdevice_queue(dev, head);
-
-	if (--vs->refcnt == 0) {
-		hlist_del_rcu(&vs->hlist);
-		schedule_work(&vs->del_work);
-	}
+	vxlan_sock_release(vs);
 }
 
 static size_t vxlan_get_size(const struct net_device *dev)

@@ -1784,8 +1761,8 @@ late_initcall(vxlan_init_module);
 
 static void __exit vxlan_cleanup_module(void)
 {
-	rtnl_link_unregister(&vxlan_link_ops);
 	unregister_pernet_device(&vxlan_net_ops);
+	rtnl_link_unregister(&vxlan_link_ops);
 	rcu_barrier();
 }
 module_exit(vxlan_cleanup_module);
Signature · 1 line
1.7.10.4
#8 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work
[   55.013309] task: ffff880070eac900 ti: ffff8800716d4000 task.ti:
ffff8800716d4000
[   55.013309] RIP: 0010:[<ffffffff815df704>]  [<ffffffff815df704>]
vxlan_sock_release+0x25/0x55
[   55.013309] RSP: 0018:ffff8800716d5cf8  EFLAGS: 00010246
[   55.013309] RAX: 0000000000000000 RBX: ffff88006fd7c000 RCX:
0000000ccee84d06
[   55.013309] RDX: dead000000200200 RSI: ffff880070ead048 RDI:
ffff88006fd7c070
[   55.013309] RBP: ffff8800716d5d08 R08: 0000000000000000 R09:
ffff8800716d5c48
[   55.013309] R10: 000000000000b6c4 R11: 000000000000b163 R12:
ffff88006ebb7400
[   55.013309] R13: ffff88006fd7c000 R14: ffff8800723b4520 R15:
0000000000000000
[   55.013309] FS:  0000000000000000(0000) GS:ffff88007f800000(0000)
knlGS:0000000000000000
[   55.013309] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   55.013309] CR2: 00007fa99dc0a000 CR3: 0000000070c17000 CR4:
00000000000006e0
[   55.013309] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[   55.013309] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[   55.013309] Stack:
[   55.013309]  ffff880070eac900 ffff880070fc0b58 ffff8800716d5d48
ffffffff815e0a73
[   55.013309]  080808e0716d5d28 0000000f00000000 ffff8800714ef008
ffff880070fc0b58
[   55.013309]  ffff88007f9d6200 ffff88007f9d38c0 ffff8800716d5de8
ffffffff8106872f
[   55.013309] Call Trace:
[   55.013309]  [<ffffffff815e0a73>] vxlan_igmp_work+0xa8/0xcf
[   55.013309]  [<ffffffff8106872f>] process_one_work+0x240/0x408
[   55.013309]  [<ffffffff81068662>] ? process_one_work+0x173/0x408
[   55.013309]  [<ffffffff81068c82>] worker_thread+0x15d/0x1f1
[   55.013309]  [<ffffffff81068b25>] ? rescuer_thread+0x1ff/0x1ff
[   55.013309]  [<ffffffff81070c19>] kthread+0xb1/0xb9
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
[   55.013309]  [<ffffffff81976bdc>] ret_from_fork+0x7c/0xb0
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
#9 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

Quoted 15 lines · Cong Wang
Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

Quoted 7 lines

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work

I think the problem happens because vxlan_dellink does unregister_netdevice_queue and then immediately calls vxlan_sock_release and thus vs_sock is released before igmp_work starts

Quoted 42 lines
[   55.013309] task: ffff880070eac900 ti: ffff8800716d4000 task.ti:
ffff8800716d4000
[   55.013309] RIP: 0010:[<ffffffff815df704>]  [<ffffffff815df704>]
vxlan_sock_release+0x25/0x55
[   55.013309] RSP: 0018:ffff8800716d5cf8  EFLAGS: 00010246
[   55.013309] RAX: 0000000000000000 RBX: ffff88006fd7c000 RCX:
0000000ccee84d06
[   55.013309] RDX: dead000000200200 RSI: ffff880070ead048 RDI:
ffff88006fd7c070
[   55.013309] RBP: ffff8800716d5d08 R08: 0000000000000000 R09:
ffff8800716d5c48
[   55.013309] R10: 000000000000b6c4 R11: 000000000000b163 R12:
ffff88006ebb7400
[   55.013309] R13: ffff88006fd7c000 R14: ffff8800723b4520 R15:
0000000000000000
[   55.013309] FS:  0000000000000000(0000) GS:ffff88007f800000(0000)
knlGS:0000000000000000
[   55.013309] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   55.013309] CR2: 00007fa99dc0a000 CR3: 0000000070c17000 CR4:
00000000000006e0
[   55.013309] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[   55.013309] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[   55.013309] Stack:
[   55.013309]  ffff880070eac900 ffff880070fc0b58 ffff8800716d5d48
ffffffff815e0a73
[   55.013309]  080808e0716d5d28 0000000f00000000 ffff8800714ef008
ffff880070fc0b58
[   55.013309]  ffff88007f9d6200 ffff88007f9d38c0 ffff8800716d5de8
ffffffff8106872f
[   55.013309] Call Trace:
[   55.013309]  [<ffffffff815e0a73>] vxlan_igmp_work+0xa8/0xcf
[   55.013309]  [<ffffffff8106872f>] process_one_work+0x240/0x408
[   55.013309]  [<ffffffff81068662>] ? process_one_work+0x173/0x408
[   55.013309]  [<ffffffff81068c82>] worker_thread+0x15d/0x1f1
[   55.013309]  [<ffffffff81068b25>] ? rescuer_thread+0x1ff/0x1ff
[   55.013309]  [<ffffffff81070c19>] kthread+0xb1/0xb9
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
[   55.013309]  [<ffffffff81976bdc>] ret_from_fork+0x7c/0xb0
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Sincerely yours,
Mike.

#10 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

Quoted 24 lines · Mike Rapoport
Quoted 15 lines · Cong Wang
Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

Quoted 7 lines

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work
Quoted 4 lines

I think the problem happens because vxlan_dellink does unregister_netdevice_queue and then immediately calls vxlan_sock_release and thus vs_sock is released before igmp_work starts

Yeah, seems something wrong with refcnt.

Thanks!

Quoted 53 lines
Quoted 43 lines
[   55.013309] task: ffff880070eac900 ti: ffff8800716d4000 task.ti:
ffff8800716d4000
[   55.013309] RIP: 0010:[<ffffffff815df704>]  [<ffffffff815df704>]
vxlan_sock_release+0x25/0x55
[   55.013309] RSP: 0018:ffff8800716d5cf8  EFLAGS: 00010246
[   55.013309] RAX: 0000000000000000 RBX: ffff88006fd7c000 RCX:
0000000ccee84d06
[   55.013309] RDX: dead000000200200 RSI: ffff880070ead048 RDI:
ffff88006fd7c070
[   55.013309] RBP: ffff8800716d5d08 R08: 0000000000000000 R09:
ffff8800716d5c48
[   55.013309] R10: 000000000000b6c4 R11: 000000000000b163 R12:
ffff88006ebb7400
[   55.013309] R13: ffff88006fd7c000 R14: ffff8800723b4520 R15:
0000000000000000
[   55.013309] FS:  0000000000000000(0000) GS:ffff88007f800000(0000)
knlGS:0000000000000000
[   55.013309] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   55.013309] CR2: 00007fa99dc0a000 CR3: 0000000070c17000 CR4:
00000000000006e0
[   55.013309] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[   55.013309] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[   55.013309] Stack:
[   55.013309]  ffff880070eac900 ffff880070fc0b58 ffff8800716d5d48
ffffffff815e0a73
[   55.013309]  080808e0716d5d28 0000000f00000000 ffff8800714ef008
ffff880070fc0b58
[   55.013309]  ffff88007f9d6200 ffff88007f9d38c0 ffff8800716d5de8
ffffffff8106872f
[   55.013309] Call Trace:
[   55.013309]  [<ffffffff815e0a73>] vxlan_igmp_work+0xa8/0xcf
[   55.013309]  [<ffffffff8106872f>] process_one_work+0x240/0x408
[   55.013309]  [<ffffffff81068662>] ? process_one_work+0x173/0x408
[   55.013309]  [<ffffffff81068c82>] worker_thread+0x15d/0x1f1
[   55.013309]  [<ffffffff81068b25>] ? rescuer_thread+0x1ff/0x1ff
[   55.013309]  [<ffffffff81070c19>] kthread+0xb1/0xb9
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
[   55.013309]  [<ffffffff81976bdc>] ret_from_fork+0x7c/0xb0
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Sincerely yours,
Mike.

author
#11 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

In reply to Mike Rapoport
Quoted 24 lines · On Wed, 5 Jun 2013 10:29:10 +0300 Mike Rapoport…
Quoted 15 lines · Cong Wang
Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

Quoted 7 lines

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work
Quoted 4 lines

I think the problem happens because vxlan_dellink does unregister_netdevice_queue and then immediately calls vxlan_sock_release and thus vs_sock is released before igmp_work starts

This is handled because a refcount is acquired before the igmp_work is scheduled.

#12 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

Quoted 27 lines · Stephen Hemminger
Quoted 24 lines · On Wed, 5 Jun 2013 10:29:10 +0300 Mike Rapoport…
Quoted 22 lines · Cong Wang
Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work
Quoted 7 lines
Quoted 4 lines

I think the problem happens because vxlan_dellink does unregister_netdevice_queue and then immediately calls vxlan_sock_release and thus vs_sock is released before igmp_work starts

This is handled because a refcount is acquired before the igmp_work is scheduled.

As far as I can tell, the vxlan_sock_release in vxlan_dellink call occurs before vxlan_stop is invoked, and therefore before refcount is acquired for igmp_work.
When I tried to use unregister_netdevice_queue(dev, NULL) I haven't seen crashes.

--
Sincerely yours,
Mike.

author
#13 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

In reply to Cong Wang
Quoted 55 lines · On Wed, 5 Jun 2013 06:47:52 +0000 (UTC) Cong Wang <[email protected]>…
Quoted 4 lines · Stephen Hemminger

Do join/leave from work queue to avoid lock inversion problems between normal socket and RTNL. The code comes out cleaner as well.

Quoted 9 lines

Uses Cong Wang's suggestion to turn refcnt into a real atomic since now need to handle case where last use of socket is IGMP worker.

Also fixes race where vxlan_stop could be called after device was deleted on module removal. The call to rtnl_link_unregister would call dellink while vxlan device was still up. Reordering the calls fixes it.

After the first 3 patches applied, I got:

[   55.010954] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[   55.013309] CPU: 1 PID: 163 Comm: kworker/1:2 Not tainted
3.10.0-rc2+ #1150
[   55.013309] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   55.013309] Workqueue: events vxlan_igmp_work
[   55.013309] task: ffff880070eac900 ti: ffff8800716d4000 task.ti:
ffff8800716d4000
[   55.013309] RIP: 0010:[<ffffffff815df704>]  [<ffffffff815df704>]
vxlan_sock_release+0x25/0x55
[   55.013309] RSP: 0018:ffff8800716d5cf8  EFLAGS: 00010246
[   55.013309] RAX: 0000000000000000 RBX: ffff88006fd7c000 RCX:
0000000ccee84d06
[   55.013309] RDX: dead000000200200 RSI: ffff880070ead048 RDI:
ffff88006fd7c070
[   55.013309] RBP: ffff8800716d5d08 R08: 0000000000000000 R09:
ffff8800716d5c48
[   55.013309] R10: 000000000000b6c4 R11: 000000000000b163 R12:
ffff88006ebb7400
[   55.013309] R13: ffff88006fd7c000 R14: ffff8800723b4520 R15:
0000000000000000
[   55.013309] FS:  0000000000000000(0000) GS:ffff88007f800000(0000)
knlGS:0000000000000000
[   55.013309] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   55.013309] CR2: 00007fa99dc0a000 CR3: 0000000070c17000 CR4:
00000000000006e0
[   55.013309] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[   55.013309] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[   55.013309] Stack:
[   55.013309]  ffff880070eac900 ffff880070fc0b58 ffff8800716d5d48
ffffffff815e0a73
[   55.013309]  080808e0716d5d28 0000000f00000000 ffff8800714ef008
ffff880070fc0b58
[   55.013309]  ffff88007f9d6200 ffff88007f9d38c0 ffff8800716d5de8
ffffffff8106872f
[   55.013309] Call Trace:
[   55.013309]  [<ffffffff815e0a73>] vxlan_igmp_work+0xa8/0xcf
Quoted 9 lines
[   55.013309]  [<ffffffff8106872f>] process_one_work+0x240/0x408
[   55.013309]  [<ffffffff81068662>] ? process_one_work+0x173/0x408
[   55.013309]  [<ffffffff81068c82>] worker_thread+0x15d/0x1f1
[   55.013309]  [<ffffffff81068b25>] ? rescuer_thread+0x1ff/0x1ff
[   55.013309]  [<ffffffff81070c19>] kthread+0xb1/0xb9
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17
[   55.013309]  [<ffffffff81976bdc>] ret_from_fork+0x7c/0xb0
[   55.013309]  [<ffffffff81070b68>] ? freezing+0x17/0x17

What was the order of events (ie. what did you do to make it happen)?

#14 lore raw

Re: [PATCH net-next 03/10] vxlan: move IGMP join/leave to work queue

Quoted 2 lines · Stephen Hemminger

What was the order of events (ie. what did you do to make it happen)?

1. generate some traffic via vxlan0 with ping
2. at the same time delete vxlan0

this is 100% reproducible.

author
#15 lore raw

PATCH 1/1 vxlan: fix crash on module removal

In reply to Stephen Hemminger

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

Signed-off-by Stephen Hemminger <[email protected]>

---
See "vxlan: move IGMP join/leave to work queue" for the net-next version of this

 drivers/net/vxlan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/vxlan.c +1 -1
index 3b1d2ee..5cc3b48 100644
@@ -1677,8 +1677,8 @@ module_init(vxlan_init_module);
 
 static void __exit vxlan_cleanup_module(void)
 {
-	rtnl_link_unregister(&vxlan_link_ops);
 	unregister_pernet_device(&vxlan_net_ops);
+	rtnl_link_unregister(&vxlan_link_ops);
 	rcu_barrier();
 }
 module_exit(vxlan_cleanup_module);
Signature · 1 line
1.7.10.4
#16 lore raw

Re: [PATCH net] vxlan: fix crash on module removal

Quoted 4 lines · Stephen Hemminger

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

Two points:

1. There are still many others drivers calling them in such order...
2. The removal should be in a reverse order of creation, at least in theory.

I mean this *might* indicate something wrong.

author
#17 lore raw

Re: [PATCH net] vxlan: fix crash on module removal

Quoted 6 lines · On Thu, 6 Jun 2013 01:11:11 +0000 (UTC) Cong Wang <[email protected]>…
Quoted 4 lines · Stephen Hemminger

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

Quoted 7 lines

Two points:

1. There are still many others drivers calling them in such order...
2. The removal should be in a reverse order of creation, at least in theory.

I mean this *might* indicate something wrong.

1. Other drivers either don't have the dependency or are broken. It makes no sense to stop a device that is deleted.

2. The normal creation order is
vxlan_setup
vxlan_newlink
vxlan_open

author
#18 lore raw

PATCH 4/10 vxlan: send notification when MAC migrates

In reply to Stephen Hemminger

When learned entry migrates to another IP send a notification that entry has changed.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |    1 +
 1 file changed, 1 insertion(+)
drivers/net/vxlan.c +1 -0
index f98f459..25d3e9e 100644
@@ -628,6 +628,7 @@ static int vxlan_snoop(struct net_device *dev,
 
 		f->remote.remote_ip = src_ip;
 		f->updated = jiffies;
+		vxlan_fdb_notify(vxlan, f, RTM_NEWNEIGH);
 	} else {
 		/* learned new entry */
 		spin_lock(&vxlan->hash_lock);
Signature · 1 line
1.7.10.4
author
#19 lore raw

PATCH 5/10 vxlan: make vxlan_xmit_one void

In reply to Stephen Hemminger

The function vxlan_xmit_one always returns NETDEV_TX_OK, so there is no point in keeping track of return values etc.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)
drivers/net/vxlan.c +9 -17
index 25d3e9e..f0b7605 100644
@@ -996,8 +996,8 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan,
 	}
 }
 
-static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
-				  struct vxlan_rdst *rdst, bool did_rsc)
+static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
+			   struct vxlan_rdst *rdst, bool did_rsc)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct rtable *rt;

@@ -1020,7 +1020,7 @@ static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (did_rsc) {
 			/* short-circuited back to local bridge */
 			vxlan_encap_bypass(skb, vxlan, vxlan);
-			return NETDEV_TX_OK;
+			return;
 		}
 		goto drop;
 	}

@@ -1076,7 +1076,7 @@ static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (!dst_vxlan)
 			goto tx_error;
 		vxlan_encap_bypass(skb, vxlan, dst_vxlan);
-		return NETDEV_TX_OK;
+		return;
 	}
 
 	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));

@@ -1120,7 +1120,7 @@ static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		goto drop;
 
 	iptunnel_xmit(skb, dev);
-	return NETDEV_TX_OK;
+	return;
 
 drop:
 	dev->stats.tx_dropped++;

@@ -1130,7 +1130,6 @@ tx_error:
 	dev->stats.tx_errors++;
 tx_free:
 	dev_kfree_skb(skb);
-	return NETDEV_TX_OK;
 }
 
 /* Transmit local packets over Vxlan

@@ -1146,7 +1145,6 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 	bool did_rsc = false;
 	struct vxlan_rdst *rdst0, *rdst;
 	struct vxlan_fdb *f;
-	int rc1, rc;
 
 	skb_reset_mac_header(skb);
 	eth = eth_hdr(skb);

@@ -1174,24 +1172,18 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 	} else
 		rdst0 = &f->remote;
 
-	rc = NETDEV_TX_OK;
 
 	/* if there are multiple destinations, send copies */
 	for (rdst = rdst0->remote_next; rdst; rdst = rdst->remote_next) {
 		struct sk_buff *skb1;
 
 		skb1 = skb_clone(skb, GFP_ATOMIC);
-		if (skb1) {
-			rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-			if (rc == NETDEV_TX_OK)
-				rc = rc1;
-		}
+		if (skb1)
+			vxlan_xmit_one(skb1, dev, rdst, did_rsc);
 	}
 
-	rc1 = vxlan_xmit_one(skb, dev, rdst0, did_rsc);
-	if (rc == NETDEV_TX_OK)
-		rc = rc1;
-	return rc;
+	vxlan_xmit_one(skb, dev, rdst0, did_rsc);
+	return NETDEV_TX_OK;
 }
 
 /* Walk the forwarding table and purge stale entries */
Signature · 1 line
1.7.10.4
Acked-by on v1 5/10
#20 lore raw

Re: [PATCH net-next 05/10] vxlan: make vxlan_xmit_one void

Acked-by David L Stevens <[email protected]>

[email protected] wrote on 06/05/2013 12:24:09 AM:

Quoted 113 lines

From: Stephen Hemminger <[email protected]>

The function vxlan_xmit_one always returns NETDEV_TX_OK, so there is no point in keeping track of return values etc.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)
drivers/net/vxlan.c +2 -2
index 25d3e9e..f0b7605 100644
@@ -996,8 +996,8 @@ static void vxlan_encap_bypass(struct sk_buff 
 *skb, struct vxlan_dev *src_vxlan,
    }
 }
 
-static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct 
 net_device *dev,
-              struct vxlan_rdst *rdst, bool did_rsc)
+static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
+            struct vxlan_rdst *rdst, bool did_rsc)
 {

    struct vxlan_dev *vxlan = netdev_priv(dev);
    struct rtable *rt;
drivers/net/vxlan.c +1 -1
@@ -1020,7 +1020,7 @@ static netdev_tx_t vxlan_xmit_one(struct 
 sk_buff *skb, struct net_device *dev,
       if (did_rsc) {
          /* short-circuited back to local bridge */
          vxlan_encap_bypass(skb, vxlan, vxlan);
-         return NETDEV_TX_OK;
+         return;
       }
       goto drop;

    }
drivers/net/vxlan.c +1 -1
@@ -1076,7 +1076,7 @@ static netdev_tx_t vxlan_xmit_one(struct 
 sk_buff *skb, struct net_device *dev,
       if (!dst_vxlan)
          goto tx_error;
       vxlan_encap_bypass(skb, vxlan, dst_vxlan);
-      return NETDEV_TX_OK;
+      return;
    }
 

    memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
drivers/net/vxlan.c +1 -1
@@ -1120,7 +1120,7 @@ static netdev_tx_t vxlan_xmit_one(struct 
 sk_buff *skb, struct net_device *dev,
       goto drop;
 
    iptunnel_xmit(skb, dev);
-   return NETDEV_TX_OK;
+   return;
 
 drop:

    dev->stats.tx_dropped++;
drivers/net/vxlan.c +0 -2
@@ -1130,7 +1130,6 @@ tx_error:
    dev->stats.tx_errors++;
 tx_free:
    dev_kfree_skb(skb);
-   return NETDEV_TX_OK;
 }
 
 /* Transmit local packets over Vxlan

@@ -1146,7 +1145,6 @@ static netdev_tx_t vxlan_xmit(struct sk_buff 
 *skb, struct net_device *dev)
    bool did_rsc = false;
    struct vxlan_rdst *rdst0, *rdst;
    struct vxlan_fdb *f;
-   int rc1, rc;
 
    skb_reset_mac_header(skb);

    eth = eth_hdr(skb);
drivers/net/vxlan.c +4 -10
@@ -1174,24 +1172,18 @@ static netdev_tx_t vxlan_xmit(struct sk_buff
 *skb, struct net_device *dev)
    } else
       rdst0 = &f->remote;
 
-   rc = NETDEV_TX_OK;
 
    /* if there are multiple destinations, send copies */
    for (rdst = rdst0->remote_next; rdst; rdst = rdst->remote_next) {
       struct sk_buff *skb1;
 
       skb1 = skb_clone(skb, GFP_ATOMIC);
-      if (skb1) {
-         rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-         if (rc == NETDEV_TX_OK)
-            rc = rc1;
-      }
+      if (skb1)
+         vxlan_xmit_one(skb1, dev, rdst, did_rsc);
    }
 
-   rc1 = vxlan_xmit_one(skb, dev, rdst0, did_rsc);
-   if (rc == NETDEV_TX_OK)
-      rc = rc1;
-   return rc;
+   vxlan_xmit_one(skb, dev, rdst0, did_rsc);
+   return NETDEV_TX_OK;
 }

/* Walk the forwarding table and purge stale entries */

Signature · 6 lines
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#21 lore raw

PATCH 6/10 vxlan: convert remotes list to list_rcu

In reply to Stephen Hemminger

Based on initial work by Mike Rapoport <[email protected]> Use list macros and RCU for tracking multiple remotes.

Note: this code assumes list always has at least one entry, because delete is not supported.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |   99 +++++++++++++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 43 deletions(-)
drivers/net/vxlan.c +56 -43
index f0b7605..593c895 100644
@@ -101,7 +101,7 @@ struct vxlan_rdst {
 	__be16			 remote_port;
 	u32			 remote_vni;
 	u32			 remote_ifindex;
-	struct vxlan_rdst	*remote_next;
+	struct list_head	 list;
 };
 
 /* Forwarding table entry */

@@ -110,7 +110,7 @@ struct vxlan_fdb {
 	struct rcu_head	  rcu;
 	unsigned long	  updated;	/* jiffies */
 	unsigned long	  used;
-	struct vxlan_rdst remote;
+	struct list_head  remotes;
 	u16		  state;	/* see ndm_state */
 	u8		  flags;	/* see ndm_flags */
 	u8		  eth_addr[ETH_ALEN];

@@ -164,6 +164,14 @@ static inline struct hlist_head *vs_head(struct net *net, __be16 port)
 	return &vn->sock_list[hash_32(ntohs(port), PORT_HASH_BITS)];
 }
 
+/* First remote destination for a forwarding entry.
+ * Guaranteed to be non-NULL because remotes are never deleted.
+ */
+static inline struct vxlan_rdst *first_remote(struct vxlan_fdb *fdb)
+{
+	return list_first_or_null_rcu(&fdb->remotes, struct vxlan_rdst, list);
+}
+
 /* Find VXLAN socket based on network namespace and UDP port */
 static struct vxlan_sock *vxlan_find_port(struct net *net, __be16 port)
 {

@@ -269,7 +277,7 @@ static inline size_t vxlan_nlmsg_size(void)
 }
 
 static void vxlan_fdb_notify(struct vxlan_dev *vxlan,
-			     const struct vxlan_fdb *fdb, int type)
+			     struct vxlan_fdb *fdb, int type)
 {
 	struct net *net = dev_net(vxlan->dev);
 	struct sk_buff *skb;

@@ -279,7 +287,7 @@ static void vxlan_fdb_notify(struct vxlan_dev *vxlan,
 	if (skb == NULL)
 		goto errout;
 
-	err = vxlan_fdb_info(skb, vxlan, fdb, 0, 0, type, 0, &fdb->remote);
+	err = vxlan_fdb_info(skb, vxlan, fdb, 0, 0, type, 0, first_remote(fdb));
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in vxlan_nlmsg_size() */
 		WARN_ON(err == -EMSGSIZE);

@@ -298,11 +306,16 @@ static void vxlan_ip_miss(struct net_device *dev, __be32 ipa)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_fdb f;
+	struct vxlan_rdst remote;
 
 	memset(&f, 0, sizeof f);
 	f.state = NUD_STALE;
-	f.remote.remote_ip = ipa; /* goes to NDA_DST */
-	f.remote.remote_vni = VXLAN_N_VID;
+
+	remote.remote_ip = ipa; /* goes to NDA_DST */
+	remote.remote_vni = VXLAN_N_VID;
+
+	INIT_LIST_HEAD(&f.remotes);
+	list_add_rcu(&remote.list, &f.remotes);
 
 	vxlan_fdb_notify(vxlan, &f, RTM_GETNEIGH);
 }

@@ -312,6 +325,7 @@ static void vxlan_fdb_miss(struct vxlan_dev *vxlan, const u8 eth_addr[ETH_ALEN])
 	struct vxlan_fdb	f;
 
 	memset(&f, 0, sizeof f);
+	INIT_LIST_HEAD(&f.remotes);
 	f.state = NUD_STALE;
 	memcpy(f.eth_addr, eth_addr, ETH_ALEN);
 

@@ -371,17 +385,17 @@ static struct vxlan_fdb *vxlan_find_mac(struct vxlan_dev *vxlan,
 static int vxlan_fdb_append(struct vxlan_fdb *f,
 			    __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
 {
-	struct vxlan_rdst *rd_prev, *rd;
+	struct vxlan_rdst *rd;
 
-	rd_prev = NULL;
-	for (rd = &f->remote; rd; rd = rd->remote_next) {
+	/* protected by vxlan->hash_lock */
+	list_for_each_entry(rd, &f->remotes, list) {
 		if (rd->remote_ip == ip &&
 		    rd->remote_port == port &&
 		    rd->remote_vni == vni &&
 		    rd->remote_ifindex == ifindex)
 			return 0;
-		rd_prev = rd;
 	}
+
 	rd = kmalloc(sizeof(*rd), GFP_ATOMIC);
 	if (rd == NULL)
 		return -ENOBUFS;

@@ -389,8 +403,9 @@ static int vxlan_fdb_append(struct vxlan_fdb *f,
 	rd->remote_port = port;
 	rd->remote_vni = vni;
 	rd->remote_ifindex = ifindex;
-	rd->remote_next = NULL;
-	rd_prev->remote_next = rd;
+
+	list_add_tail_rcu(&rd->list, &f->remotes);
+
 	return 1;
 }
 

@@ -442,16 +457,14 @@ static int vxlan_fdb_create(struct vxlan_dev *vxlan,
 			return -ENOMEM;
 
 		notify = 1;
-		f->remote.remote_ip = ip;
-		f->remote.remote_port = port;
-		f->remote.remote_vni = vni;
-		f->remote.remote_ifindex = ifindex;
-		f->remote.remote_next = NULL;
 		f->state = state;
 		f->flags = ndm_flags;
 		f->updated = f->used = jiffies;
+		INIT_LIST_HEAD(&f->remotes);
 		memcpy(f->eth_addr, mac, ETH_ALEN);
 
+		vxlan_fdb_append(f, ip, port, vni, ifindex);
+
 		++vxlan->addrcnt;
 		hlist_add_head_rcu(&f->hlist,
 				   vxlan_fdb_head(vxlan, mac));

@@ -466,13 +479,10 @@ static int vxlan_fdb_create(struct vxlan_dev *vxlan,
 static void vxlan_fdb_free(struct rcu_head *head)
 {
 	struct vxlan_fdb *f = container_of(head, struct vxlan_fdb, rcu);
+	struct vxlan_rdst *rd, *nd;
 
-	while (f->remote.remote_next) {
-		struct vxlan_rdst *rd = f->remote.remote_next;
-
-		f->remote.remote_next = rd->remote_next;
+	list_for_each_entry_safe(rd, nd, &f->remotes, list)
 		kfree(rd);
-	}
 	kfree(f);
 }
 

@@ -582,23 +592,24 @@ static int vxlan_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 
 		hlist_for_each_entry_rcu(f, &vxlan->fdb_head[h], hlist) {
 			struct vxlan_rdst *rd;
-			for (rd = &f->remote; rd; rd = rd->remote_next) {
-				if (idx < cb->args[0])
-					goto skip;
 
+			if (idx < cb->args[0])
+				goto skip;
+
+			list_for_each_entry_rcu(rd, &f->remotes, list) {
 				err = vxlan_fdb_info(skb, vxlan, f,
 						     NETLINK_CB(cb->skb).portid,
 						     cb->nlh->nlmsg_seq,
 						     RTM_NEWNEIGH,
 						     NLM_F_MULTI, rd);
 				if (err < 0)
-					break;
-skip:
-				++idx;
+					goto out;
 			}
+skip:
+			++idx;
 		}
 	}
-
+out:
 	return idx;
 }
 

@@ -614,19 +625,21 @@ static int vxlan_snoop(struct net_device *dev,
 
 	f = vxlan_find_mac(vxlan, src_mac);
 	if (likely(f)) {
-		if (likely(f->remote.remote_ip == src_ip))
-			return 0;
+		struct vxlan_rdst *remote = first_remote(f);
 
 		/* Don't migrate static entries, drop packets */
 		if (!(f->flags & NTF_SELF))
 			return 1;
 
+		if (likely(remote->remote_ip == src_ip))
+			return 0;
+
 		if (net_ratelimit())
 			netdev_info(dev,
 				    "%pM migrated from %pI4 to %pI4\n",
-				    src_mac, &f->remote.remote_ip, &src_ip);
+				    src_mac, &remote->remote_ip, &src_ip);
 
-		f->remote.remote_ip = src_ip;
+		remote->remote_ip = src_ip;
 		f->updated = jiffies;
 		vxlan_fdb_notify(vxlan, f, RTM_NEWNEIGH);
 	} else {

@@ -854,7 +867,7 @@ static int arp_reduce(struct net_device *dev, struct sk_buff *skb)
 		}
 
 		f = vxlan_find_mac(vxlan, n->ha);
-		if (f && f->remote.remote_ip == htonl(INADDR_ANY)) {
+		if (f && first_remote(f)->remote_ip == htonl(INADDR_ANY)) {
 			/* bridge-local neighbor */
 			neigh_release(n);
 			goto out;

@@ -1169,17 +1182,17 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 		    (vxlan->flags & VXLAN_F_L2MISS) &&
 		    !is_multicast_ether_addr(eth->h_dest))
 			vxlan_fdb_miss(vxlan, eth->h_dest);
-	} else
-		rdst0 = &f->remote;
-
+	} else {
+		rdst = rdst0 = first_remote(f);
 
-	/* if there are multiple destinations, send copies */
-	for (rdst = rdst0->remote_next; rdst; rdst = rdst->remote_next) {
-		struct sk_buff *skb1;
+		/* if there are multiple destinations, send copies */
+		list_for_each_entry_continue_rcu(rdst, &f->remotes, list) {
+			struct sk_buff *skb1;
 
-		skb1 = skb_clone(skb, GFP_ATOMIC);
-		if (skb1)
-			vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+			skb1 = skb_clone(skb, GFP_ATOMIC);
+			if (skb1)
+				vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+		}
 	}
 
 	vxlan_xmit_one(skb, dev, rdst0, did_rsc);
Signature · 1 line
1.7.10.4
author
#22 lore raw

PATCH 7/10 vxlan: port module param should be ushort

In reply to Stephen Hemminger

UDP ports's are limited to 16 bits.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/vxlan.c +2 -2
index 593c895..57b5274 100644
@@ -70,8 +70,8 @@ struct vxlanhdr {
  * The IANA assigned port is 4789, but the Linux default is 8472
  * for compatability with early adopters.
  */
-static unsigned int vxlan_port __read_mostly = 8472;
-module_param_named(udp_port, vxlan_port, uint, 0444);
+static unsigned short vxlan_port __read_mostly = 8472;
+module_param_named(udp_port, vxlan_port, ushort, 0444);
 MODULE_PARM_DESC(udp_port, "Destination UDP port");
 
 static bool log_ecn_error = true;
Signature · 1 line
1.7.10.4
#23 lore raw

Re: [PATCH net-next 07/10] vxlan: port module param should be ushort

Hello.

Quoted 1 line · Stephen Hemminger

UDP ports's are limited to 16 bits.

s/ports's/ports/. Maybe a committer can fix.

Quoted 1 line
Signed-off-by Stephen Hemminger <[email protected]>

WBR, Sergei

author
#24 lore raw

PATCH 8/10 vxlan: use initializer for dummy structures

In reply to Stephen Hemminger

For the notification code, a couple of places build fdb entries on the stack, use structure initialization instead and fix formatting.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/net/vxlan.c +10 -11
index 57b5274..664381a 100644
@@ -305,14 +305,13 @@ errout:
 static void vxlan_ip_miss(struct net_device *dev, __be32 ipa)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct vxlan_fdb f;
-	struct vxlan_rdst remote;
-
-	memset(&f, 0, sizeof f);
-	f.state = NUD_STALE;
-
-	remote.remote_ip = ipa; /* goes to NDA_DST */
-	remote.remote_vni = VXLAN_N_VID;
+	struct vxlan_fdb f = {
+		.state = NUD_STALE,
+	};
+	struct vxlan_rdst remote = {
+		.remote_ip = ipa, /* goes to NDA_DST */
+		.remote_vni = VXLAN_N_VID,
+	};
 
 	INIT_LIST_HEAD(&f.remotes);
 	list_add_rcu(&remote.list, &f.remotes);

@@ -322,11 +321,11 @@ static void vxlan_ip_miss(struct net_device *dev, __be32 ipa)
 
 static void vxlan_fdb_miss(struct vxlan_dev *vxlan, const u8 eth_addr[ETH_ALEN])
 {
-	struct vxlan_fdb	f;
+	struct vxlan_fdb f = {
+		.state = NUD_STALE,
+	};
 
-	memset(&f, 0, sizeof f);
 	INIT_LIST_HEAD(&f.remotes);
-	f.state = NUD_STALE;
 	memcpy(f.eth_addr, eth_addr, ETH_ALEN);
 
 	vxlan_fdb_notify(vxlan, &f, RTM_GETNEIGH);
Signature · 1 line
1.7.10.4
author
#25 lore raw

PATCH 9/10 vxlan: whitespace cleanup

In reply to Stephen Hemminger
Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/vxlan.c +4 -4
index 664381a..a3690d4 100644
@@ -204,9 +204,9 @@ static struct vxlan_dev *vxlan_find_vni(struct net *net, u32 id, __be16 port)
 
 /* Fill in neighbour message in skbuff. */
 static int vxlan_fdb_info(struct sk_buff *skb, struct vxlan_dev *vxlan,
-			   const struct vxlan_fdb *fdb,
-			   u32 portid, u32 seq, int type, unsigned int flags,
-			   const struct vxlan_rdst *rdst)
+			  const struct vxlan_fdb *fdb,
+			  u32 portid, u32 seq, int type, unsigned int flags,
+			  const struct vxlan_rdst *rdst)
 {
 	unsigned long now = jiffies;
 	struct nda_cacheinfo ci;

@@ -1020,7 +1020,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	struct flowi4 fl4;
 	__be32 dst;
 	__be16 src_port, dst_port;
-        u32 vni;
+	u32 vni;
 	__be16 df = 0;
 	__u8 tos, ttl;
Signature · 1 line
1.7.10.4
author
#27 lore raw

PATCH 10/10 vxlan: version 0.2

In reply to Stephen Hemminger
Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/vxlan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/vxlan.c +1 -1
index a3690d4..61f04b6 100644
@@ -42,7 +42,7 @@
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 
-#define VXLAN_VERSION	"0.1"
+#define VXLAN_VERSION	"0.2"
 
 #define PORT_HASH_BITS	8
 #define PORT_HASH_SIZE  (1<<PORT_HASH_BITS)
Signature · 1 line
1.7.10.4
maintainer
#29 lore raw
In reply to Stephen Hemminger

Stephen please resolve Cong Wang's crashes and then take care of the "return bool" and typo nit picks while you're at it, and I'll apply this series it looks good otherwise.

Thanks!