[Patch iproute2] vxlan: use 8472 as default dst port

[Patch iproute2] vxlan: use 8472 as default dst port

[Cong Wang]

Because Linux kernel uses the same default, otherwise
the default config doesn't work.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 2d93ee2..a0c90b3 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -53,7 +53,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u8 noage = 0;
 	__u32 age = 0;
 	__u32 maxaddr = 0;
-	__u16 dstport = 4789;
+	__u16 dstport = 8472;
 	struct ifla_vxlan_port_range range = { 0, 0 };
 
 	while (argc > 0) {
@@ -188,8 +188,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (range.low || range.high)
 		addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
 			  &range, sizeof(range));
-	if (dstport)
-		addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));
+	addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));
 
 	return 0;
 }

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Stevens]

netdev-owner@vger.kernel.org wrote on 05/15/2013 07:11:10 AM:

> From: Cong Wang <amwang@redhat.com>

> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> ---
> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> index 2d93ee2..a0c90b3 100644
> --- a/ip/iplink_vxlan.c
> +++ b/ip/iplink_vxlan.c
> @@ -53,7 +53,7 @@ static int vxlan_parse_opt(struct link_util *lu, 
> int argc, char **argv,
>     __u8 noage = 0;
>     __u32 age = 0;
>     __u32 maxaddr = 0;
> -   __u16 dstport = 4789;
> +   __u16 dstport = 8472;

I think this should be "0"; then it will use the kernel port,
whatever it is.

> @@ -188,8 +188,7 @@ static int vxlan_parse_opt(struct link_util *lu,
> int argc, char **argv,
>     if (range.low || range.high)
>        addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
>             &range, sizeof(range));
> -   if (dstport)
> -      addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));
> +   addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));

        ...and these lines should be left alone. If not specified,
it won't pass an alternate port and will use the kernel default.

                                                +-DLS

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[Stephen Hemminger]

On Wed, 15 May 2013 07:32:28 -0400
David Stevens <dlstevens@us.ibm.com> wrote:

> netdev-owner@vger.kernel.org wrote on 05/15/2013 07:11:10 AM:
> 
> > From: Cong Wang <amwang@redhat.com>
> 
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> > 
> > ---
> > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> > index 2d93ee2..a0c90b3 100644
> > --- a/ip/iplink_vxlan.c
> > +++ b/ip/iplink_vxlan.c
> > @@ -53,7 +53,7 @@ static int vxlan_parse_opt(struct link_util *lu, 
> > int argc, char **argv,
> >     __u8 noage = 0;
> >     __u32 age = 0;
> >     __u32 maxaddr = 0;
> > -   __u16 dstport = 4789;
> > +   __u16 dstport = 8472;
> 
> I think this should be "0"; then it will use the kernel port,
> whatever it is.
> 
> > @@ -188,8 +188,7 @@ static int vxlan_parse_opt(struct link_util *lu,
> > int argc, char **argv,
> >     if (range.low || range.high)
> >        addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
> >             &range, sizeof(range));
> > -   if (dstport)
> > -      addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));
> > +   addattr16(n, 1024, IFLA_VXLAN_PORT, htons(dstport));
> 
>         ...and these lines should be left alone. If not specified,
> it won't pass an alternate port and will use the kernel default.
> 
>                                                 +-DLS
> 
> 

No. I want all users to get the IANA assigned port unless they specifically
override it. For compatibility kernel can't change to the IANA value
but user space needs to nudge users in the correct direction.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Stevens]

netdev-owner@vger.kernel.org wrote on 05/15/2013 12:53:22 PM:

> From: Stephen Hemminger <stephen@networkplumber.org>
 
> No. I want all users to get the IANA assigned port unless they 
specifically
> override it. For compatibility kernel can't change to the IANA value
> but user space needs to nudge users in the correct direction.

It's more than a nudge -- it doesn't interoperate with itself.

1) The kernel binds to 8472 on module load.
2) After it's bound, iproute2 "changes" the port to 4789, but
        the kernel binding is still 8742 for listening. All destinations 
use
        the new port, but all bindings use the old port.

So, boot two systems without specifying a port and they can't
talk to each other. They both listen on 8742 and they both send to
4789.

This clearly is worse than changing the kernel default port to 4789, but
what's the point of having a kernel default if a user-level program 
overrides
it even when the user did not specify a port, and only for half of it (the
sender side)?

You could make a port change close and rebind, but that's little different
than changing the default in the kernel, if "ip" does it without a port
specification. 
                                                                +-DLS

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[Stephen Hemminger]

On Wed, 15 May 2013 13:19:43 -0400
David Stevens <dlstevens@us.ibm.com> wrote:

> netdev-owner@vger.kernel.org wrote on 05/15/2013 12:53:22 PM:
> 
> > From: Stephen Hemminger <stephen@networkplumber.org>
>  
> > No. I want all users to get the IANA assigned port unless they 
> specifically
> > override it. For compatibility kernel can't change to the IANA value
> > but user space needs to nudge users in the correct direction.
> 
> It's more than a nudge -- it doesn't interoperate with itself.
> 
> 1) The kernel binds to 8472 on module load.
> 2) After it's bound, iproute2 "changes" the port to 4789, but
>         the kernel binding is still 8742 for listening. All destinations 
> use
>         the new port, but all bindings use the old port.
> 
> So, boot two systems without specifying a port and they can't
> talk to each other. They both listen on 8742 and they both send to
> 4789.
> 
> This clearly is worse than changing the kernel default port to 4789, but
> what's the point of having a kernel default if a user-level program 
> overrides
> it even when the user did not specify a port, and only for half of it (the
> sender side)?
> 
> You could make a port change close and rebind, but that's little different
> than changing the default in the kernel, if "ip" does it without a port
> specification. 
>                                                                 +-DLS
> 

I want all new users to get the IANA port without asking.
Old users with existing nets have to force the port to the one they were using.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Stevens]

netdev-owner@vger.kernel.org wrote on 05/15/2013 01:43:08 PM:

> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> I want all new users to get the IANA port without asking.
> Old users with existing nets have to force the port to the one they 
> were using.

That'd be great -- what we're getting is all new users are getting
a listen port of 8472 and a send port of 4789. So new users who aren't
aware of why will simply see that two *new* installations don't talk
to each other (and never will, using default ports, because the kernel
and ip will need to be compatible with their incompatible selves).

You have to either specify "udp_port=4789" at modprobe or boot, or specify
a port for every fdb entry via "ip" so that the listen and send ports 
match.

I think both of those are worse than using the "wrong" port by default, 
and both
of those are worse than changing the default port for the limited user
set so far.

                                                        +-DLS

RE: [Patch iproute2] vxlan: use 8472 as default dst port

[David Laight]

> > From: Stephen Hemminger <stephen@networkplumber.org>
> >
> > I want all new users to get the IANA port without asking.
> > Old users with existing nets have to force the port to the one they
> > were using.
> 
> That'd be great -- what we're getting is all new users are getting
> a listen port of 8472 and a send port of 4789. So new users who aren't
> aware of why will simply see that two *new* installations don't talk
> to each other (and never will, using default ports, because the kernel
> and ip will need to be compatible with their incompatible selves).

Could the kernel listen on both port numbers?
Then the default 'send' port could be changed while maintaining
functionality for existing systems.

At some later time the listener on 8372 could be removed.

	David

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[Stephen Hemminger]

On Thu, 16 May 2013 09:24:47 +0100
"David Laight" <David.Laight@ACULAB.COM> wrote:

> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > >
> > > I want all new users to get the IANA port without asking.
> > > Old users with existing nets have to force the port to the one they
> > > were using.
> > 
> > That'd be great -- what we're getting is all new users are getting
> > a listen port of 8472 and a send port of 4789. So new users who aren't
> > aware of why will simply see that two *new* installations don't talk
> > to each other (and never will, using default ports, because the kernel
> > and ip will need to be compatible with their incompatible selves).
> 
> Could the kernel listen on both port numbers?
> Then the default 'send' port could be changed while maintaining
> functionality for existing systems.
> 
> At some later time the listener on 8372 could be removed.
> 
> 	David
> 
> 
> 

Actually with the destination port per vxlan it is possible to do.

1. Create vxlan0 with old port number
2. Create vxlan1 with new port number
3. Create a bridge vxlanbr0
4. Put both vxlan's into the bridge

Then the bridge will silently handle finding the correct port
and allow migrating other hosts to the new port number.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Wed, 15 May 2013 10:43:08 -0700

> I want all new users to get the IANA port without asking.
> Old users with existing nets have to force the port to the one they were using.

That's breaking things for existing users, you can't do it.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[Stephen Hemminger]

On Wed, 15 May 2013 13:19:43 -0400
David Stevens <dlstevens@us.ibm.com> wrote:

> 1) The kernel binds to 8472 on module load.
> 2) After it's bound, iproute2 "changes" the port to 4789, but
>         the kernel binding is still 8742 for listening. All destinations 
> use
>         the new port, but all bindings use the old port.

Sorry, this is a kernel bug.
Kernel shouldn't bind on module load, needs to do it later when
device is created. I will fix.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Stevens]

netdev-owner@vger.kernel.org wrote on 05/15/2013 01:48:25 PM:

> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> Sorry, this is a kernel bug.
> Kernel shouldn't bind on module load, needs to do it later when
> device is created. I will fix.

Sorry, didn't see this before last send, and fine, but if
ip changes the port for listens too, then existing users will
still get a new port by default, as soon as they get a new
"iproute2" (instead of as soon as they get a new kernel). But filter
rules and scripts that "know" the old port still won't work -
how is this better than changing the kernel default?

I think in either case, if you care what port it is, you
should specify it explicitly, always. Whether iproute2 or the
kernel makes it different, it'll break (or not) in exactly the same
ways for those who didn't.

                                                +-DLS

[RFT] vxlan: listen on multiple ports

[Stephen Hemminger]

The earlier change to introduce per-vxlan UDP port configuration did only
half the necessary work. It added per vxlan destination for sending, but
overlooked the handling of multiple ports for incoming, which means it
is broken.

This patch changes the listening port management to handle multiple
incoming UDP ports. The earlier per-namespace structure is now a hash
list per namespace.

It is also now possible to define the same virtual network id
but with different UDP port values.

A lot of the change is cosmetic like renaming vxlan_net to vxlan_sock,
and changing standard local variable from vn to vs.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
Compiles and creates/deletes vxlans and creates/deletes sockets,
but still needs more testing before going to -net

--- a/drivers/net/vxlan.c	2013-05-02 14:30:52.230572667 -0700
+++ b/drivers/net/vxlan.c	2013-05-15 14:31:54.633656522 -0700
@@ -42,8 +42,10 @@
 #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)
 #define VNI_HASH_BITS	10
 #define VNI_HASH_SIZE	(1<<VNI_HASH_BITS)
 #define FDB_HASH_BITS	8
@@ -76,13 +78,22 @@ static bool log_ecn_error = true;
 module_param(log_ecn_error, bool, 0644);
 MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
 
-/* per-net private data for this module */
 static unsigned int vxlan_net_id;
-struct vxlan_net {
-	struct socket	  *sock;	/* UDP encap socket */
+
+/* per UDP socket information */
+struct vxlan_sock {
+	struct hlist_node hlist;
+	struct rcu_head	  rcu;
+	unsigned int	  refcnt;
+	struct socket	  *sock;
 	struct hlist_head vni_list[VNI_HASH_SIZE];
 };
 
+/* per-network namespace private data for this module */
+struct vxlan_net {
+	struct hlist_head sock_list[PORT_HASH_SIZE];
+};
+
 struct vxlan_rdst {
 	struct rcu_head		 rcu;
 	__be32			 remote_ip;
@@ -107,6 +118,7 @@ struct vxlan_fdb {
 /* Pseudo network device */
 struct vxlan_dev {
 	struct hlist_node hlist;
+	struct vxlan_sock *vn_sock;
 	struct net_device *dev;
 	struct vxlan_rdst default_dst;	/* default destination */
 	__be32		  saddr;	/* source address */
@@ -135,19 +147,41 @@ struct vxlan_dev {
 /* salt for hash table */
 static u32 vxlan_salt __read_mostly;
 
-static inline struct hlist_head *vni_head(struct net *net, u32 id)
+static inline struct hlist_head *vni_head(struct vxlan_sock *vs, u32 id)
+{
+	return &vs->vni_list[hash_32(id, VNI_HASH_BITS)];
+}
+
+static inline struct hlist_head *vs_head(struct net *net, __be16 port)
+{
+	struct vxlan_net *vns = net_generic(net, vxlan_net_id);
+
+	return &vns->sock_list[hash_32(ntohs(port), PORT_HASH_BITS];
+}
+
+/* Look up a VXLAN net based on network namespace and UDP port */
+static struct vxlan_sock *vxlan_find_port(struct net *net, __be16 port)
 {
-	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
+	struct vxlan_sock *vs;
 
-	return &vn->vni_list[hash_32(id, VNI_HASH_BITS)];
+	hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
+		if (inet_sk(vs->sock->sk)->inet_dport == port)
+			return vs;
+	}
+	return NULL;
 }
 
 /* Look up VNI in a per net namespace table */
-static struct vxlan_dev *vxlan_find_vni(struct net *net, u32 id)
+static struct vxlan_dev *vxlan_find_vni(struct net *net, u32 id, __be16 port)
 {
+	struct vxlan_sock *vs;
 	struct vxlan_dev *vxlan;
 
-	hlist_for_each_entry_rcu(vxlan, vni_head(net, id), hlist) {
+	vs = vxlan_find_port(net, port);
+	if (!vs)
+		return NULL;
+
+	hlist_for_each_entry_rcu(vxlan, vni_head(vs, id), hlist) {
 		if (vxlan->default_dst.remote_vni == id)
 			return vxlan;
 	}
@@ -589,14 +623,14 @@ static void vxlan_snoop(struct net_devic
 
 
 /* See if multicast group is already in use by other ID */
-static bool vxlan_group_used(struct vxlan_net *vn,
+static bool vxlan_group_used(struct vxlan_sock *vs,
 			     const struct vxlan_dev *this)
 {
 	const struct vxlan_dev *vxlan;
 	unsigned h;
 
 	for (h = 0; h < VNI_HASH_SIZE; ++h)
-		hlist_for_each_entry(vxlan, &vn->vni_list[h], hlist) {
+		hlist_for_each_entry(vxlan, &vs->vni_list[h], hlist) {
 			if (vxlan == this)
 				continue;
 
@@ -614,8 +648,8 @@ static bool vxlan_group_used(struct vxla
 static int vxlan_join_group(struct net_device *dev)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
-	struct sock *sk = vn->sock->sk;
+	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,
@@ -623,7 +657,7 @@ static int vxlan_join_group(struct net_d
 	int err;
 
 	/* Already a member of group */
-	if (vxlan_group_used(vn, vxlan))
+	if (vxlan_group_used(vs, vxlan))
 		return 0;
 
 	/* Need to drop RTNL to call multicast join */
@@ -641,16 +675,16 @@ static int vxlan_join_group(struct net_d
 static int vxlan_leave_group(struct net_device *dev)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
+	struct vxlan_sock *vs = vxlan->vn_sock;
 	int err = 0;
-	struct sock *sk = vn->sock->sk;
+	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))
+	if (vxlan_group_used(vs, vxlan))
 		return 0;
 
 	/* Need to drop RTNL to call multicast leave */
@@ -693,7 +727,8 @@ static int vxlan_udp_encap_recv(struct s
 
 	/* Is this VNI defined? */
 	vni = ntohl(vxh->vx_vni) >> 8;
-	vxlan = vxlan_find_vni(sock_net(sk), vni);
+	vxlan = vxlan_find_vni(sock_net(skb->sk), vni,
+			       inet_sk(sk)->inet_dport);
 	if (!vxlan) {
 		netdev_dbg(skb->dev, "unknown vni %d\n", vni);
 		goto drop;
@@ -883,8 +918,8 @@ static void vxlan_sock_free(struct sk_bu
 /* On transmit, associate with the tunnel socket */
 static void vxlan_set_owner(struct net_device *dev, struct sk_buff *skb)
 {
-	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
-	struct sock *sk = vn->sock->sk;
+	struct vxlan_dev *vxlan = netdev_priv(dev);
+	struct sock *sk = vxlan->vn_sock->sock->sk;
 
 	skb_orphan(skb);
 	sock_hold(sk);
@@ -1031,7 +1066,7 @@ static netdev_tx_t vxlan_xmit_one(struct
 		struct vxlan_dev *dst_vxlan;
 
 		ip_rt_put(rt);
-		dst_vxlan = vxlan_find_vni(dev_net(dev), vni);
+		dst_vxlan = vxlan_find_vni(dev_net(dev), vni, dst_port);
 		if (!dst_vxlan)
 			goto tx_error;
 		vxlan_encap_bypass(skb, vxlan, dst_vxlan);
@@ -1390,11 +1425,77 @@ static const struct ethtool_ops vxlan_et
 	.get_link	= ethtool_op_get_link,
 };
 
+/* Create new listen socket if needed */
+static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
+{
+	struct vxlan_sock *vs;
+	struct sock *sk;
+	struct sockaddr_in vxlan_addr = {
+		.sin_family = AF_INET,
+		.sin_addr.s_addr = htonl(INADDR_ANY),
+	};
+	int rc;
+	unsigned h;
+
+	vs = kmalloc(sizeof(*vs), GFP_KERNEL);
+	if (!vs)
+		return ERR_PTR(-ENOMEM);
+
+	for (h = 0; h < VNI_HASH_SIZE; ++h)
+		INIT_HLIST_HEAD(&vs->vni_list[h]);
+
+	/* Create UDP socket for encapsulation receive. */
+	rc = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_UDP, &vs->sock);
+	if (rc < 0) {
+		pr_debug("UDP socket create failed\n");
+		kfree(vs);
+		return ERR_PTR(rc);
+	}
+
+	/* Put in proper namespace */
+	sk = vs->sock->sk;
+	sk_change_net(sk, net);
+
+	vxlan_addr.sin_port = port;
+
+	rc = kernel_bind(vs->sock, (struct sockaddr *) &vxlan_addr,
+			 sizeof(vxlan_addr));
+	if (rc < 0) {
+		pr_debug("bind for UDP socket %pI4:%u (%d)\n",
+			 &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc);
+		sk_release_kernel(sk);
+		kfree(vs);
+		return ERR_PTR(rc);
+	}
+
+	/* Disable multicast loopback */
+	inet_sk(sk)->mc_loop = 0;
+
+	/* Mark socket as an encapsulation socket. */
+	udp_sk(sk)->encap_type = 1;
+	udp_sk(sk)->encap_rcv = vxlan_udp_encap_recv;
+	udp_encap_enable();
+
+	vs->refcnt = 1;
+	return vs;
+}
+
+static void vxlan_socket_destroy(struct vxlan_sock *vs)
+{
+	ASSERT_RTNL();
+
+	hlist_del_rcu(&vs->hlist);
+
+	sk_release_kernel(vs->sock->sk);
+	kfree_rcu(vs, rcu);
+}
+
 static int vxlan_newlink(struct net *net, struct net_device *dev,
 			 struct nlattr *tb[], struct nlattr *data[])
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_rdst *dst = &vxlan->default_dst;
+	struct vxlan_sock *vs;
 	__u32 vni;
 	int err;
 
@@ -1402,10 +1503,6 @@ static int vxlan_newlink(struct net *net
 		return -EINVAL;
 
 	vni = nla_get_u32(data[IFLA_VXLAN_ID]);
-	if (vxlan_find_vni(net, vni)) {
-		pr_info("duplicate VNI %u\n", vni);
-		return -EEXIST;
-	}
 	dst->remote_vni = vni;
 
 	if (data[IFLA_VXLAN_GROUP])
@@ -1471,22 +1568,48 @@ static int vxlan_newlink(struct net *net
 	if (data[IFLA_VXLAN_PORT])
 		vxlan->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
 
+	if (vxlan_find_vni(net, vni, vxlan->dst_port)) {
+		pr_info("duplicate VNI %u\n", vni);
+		return -EEXIST;
+	}
+
+	vs = vxlan_find_port(net, vxlan->dst_port);
+	if (vs)
+		++vs->refcnt;
+	else {
+		rtnl_unlock();
+		vs = vxlan_socket_create(net, vxlan->dst_port);
+		rtnl_lock();
+		if (IS_ERR(vs))
+			return PTR_ERR(vs);
+		hlist_add_head_rcu(&vs->hlist, vs_head(net, vxlan->dst_port));
+	}
+	vxlan->vn_sock = vs;
+
 	SET_ETHTOOL_OPS(dev, &vxlan_ethtool_ops);
 
 	err = register_netdevice(dev);
-	if (!err)
-		hlist_add_head_rcu(&vxlan->hlist, vni_head(net, dst->remote_vni));
+	if (err) {
+		if (--vs->refcnt == 0)
+			vxlan_socket_destroy(vs);
+		return err;
+	}
 
-	return err;
+	hlist_add_head_rcu(&vxlan->hlist,vni_head(vs, vni));
+
+	return 0;
 }
 
 static void vxlan_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
+	struct vxlan_sock *vs = vxlan->vn_sock;
 
 	hlist_del_rcu(&vxlan->hlist);
-
 	unregister_netdevice_queue(dev, head);
+
+	if (--vs->refcnt == 0)
+		vxlan_socket_destroy(vs);
 }
 
 static size_t vxlan_get_size(const struct net_device *dev)
@@ -1571,67 +1694,31 @@ static struct rtnl_link_ops vxlan_link_o
 
 static __net_init int vxlan_init_net(struct net *net)
 {
-	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
-	struct sock *sk;
-	struct sockaddr_in vxlan_addr = {
-		.sin_family = AF_INET,
-		.sin_addr.s_addr = htonl(INADDR_ANY),
-	};
-	int rc;
+	struct vxlan_net *vns = net_generic(net, vxlan_net_id);
 	unsigned h;
 
-	/* Create UDP socket for encapsulation receive. */
-	rc = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_UDP, &vn->sock);
-	if (rc < 0) {
-		pr_debug("UDP socket create failed\n");
-		return rc;
-	}
-	/* Put in proper namespace */
-	sk = vn->sock->sk;
-	sk_change_net(sk, net);
-
-	vxlan_addr.sin_port = htons(vxlan_port);
-
-	rc = kernel_bind(vn->sock, (struct sockaddr *) &vxlan_addr,
-			 sizeof(vxlan_addr));
-	if (rc < 0) {
-		pr_debug("bind for UDP socket %pI4:%u (%d)\n",
-			 &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc);
-		sk_release_kernel(sk);
-		vn->sock = NULL;
-		return rc;
-	}
-
-	/* Disable multicast loopback */
-	inet_sk(sk)->mc_loop = 0;
-
-	/* Mark socket as an encapsulation socket. */
-	udp_sk(sk)->encap_type = 1;
-	udp_sk(sk)->encap_rcv = vxlan_udp_encap_recv;
-	udp_encap_enable();
-
-	for (h = 0; h < VNI_HASH_SIZE; ++h)
-		INIT_HLIST_HEAD(&vn->vni_list[h]);
+	for (h = 0; h < PORT_HASH_SIZE; ++h)
+		INIT_HLIST_HEAD(&vns->sock_list[h]);
 
 	return 0;
 }
 
 static __net_exit void vxlan_exit_net(struct net *net)
 {
-	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
+	struct vxlan_net *vns = net_generic(net, vxlan_net_id);
+	struct vxlan_sock *vs;
 	struct vxlan_dev *vxlan;
-	unsigned h;
+	unsigned n, h;
 
 	rtnl_lock();
-	for (h = 0; h < VNI_HASH_SIZE; ++h)
-		hlist_for_each_entry(vxlan, &vn->vni_list[h], hlist)
-			dev_close(vxlan->dev);
+	for (n = 0; n < PORT_HASH_SIZE; ++n)
+		hlist_for_each_entry(vs, &vns->sock_list[n], hlist) {
+			for (h = 0; h < VNI_HASH_SIZE; ++h)
+				hlist_for_each_entry(vxlan, &vs->vni_list[h],
+						     hlist)
+					dev_close(vxlan->dev);
+		}
 	rtnl_unlock();
-
-	if (vn->sock) {
-		sk_release_kernel(vn->sock->sk);
-		vn->sock = NULL;
-	}
 }
 
 static struct pernet_operations vxlan_net_ops = {

Re: [RFT] vxlan: listen on multiple ports

[Cong Wang]

On Wed, 2013-05-15 at 14:37 -0700, Stephen Hemminger wrote:
> The earlier change to introduce per-vxlan UDP port configuration did only
> half the necessary work. It added per vxlan destination for sending, but
> overlooked the handling of multiple ports for incoming, which means it
> is broken.
> 
> This patch changes the listening port management to handle multiple
> incoming UDP ports. The earlier per-namespace structure is now a hash
> list per namespace.
> 
> It is also now possible to define the same virtual network id
> but with different UDP port values.
> 

Besides fixing the default port issue, what other benefits does it have?
This seems overkill.

It looks like you make one socket per port, after my IPv6 patches, we
would have two sockets per port...

Re: [RFT] vxlan: listen on multiple ports

[Stephen Hemminger]

On Thu, 16 May 2013 10:51:13 +0800
Cong Wang <amwang@redhat.com> wrote:

> On Wed, 2013-05-15 at 14:37 -0700, Stephen Hemminger wrote:
> > The earlier change to introduce per-vxlan UDP port configuration did only
> > half the necessary work. It added per vxlan destination for sending, but
> > overlooked the handling of multiple ports for incoming, which means it
> > is broken.
> > 
> > This patch changes the listening port management to handle multiple
> > incoming UDP ports. The earlier per-namespace structure is now a hash
> > list per namespace.
> > 
> > It is also now possible to define the same virtual network id
> > but with different UDP port values.
> > 
> 
> Besides fixing the default port issue, what other benefits does it have?
> This seems overkill.
> 
> It looks like you make one socket per port, after my IPv6 patches, we
> would have two sockets per port...
> 
> 

The destination port option is broken now. It only sets the port
for outgoing packets, it does not change port used for incoming packets.
This change is to address that. Sorry, it shouldn't be a big
impact for IPv6.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Wed, 15 May 2013 09:53:22 -0700

> No. I want all users to get the IANA assigned port unless they specifically
> override it. For compatibility kernel can't change to the IANA value
> but user space needs to nudge users in the correct direction.

You're changing the behavior people get by invoking the tools the way
they were doing so previously.

Therefore, just as we can't change the kernel's default, you really
can't legitimately change iproute2's either.

Re: [Patch iproute2] vxlan: use 8472 as default dst port

[Stephen Hemminger]

On Wed, 15 May 2013 13:22:13 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Wed, 15 May 2013 09:53:22 -0700
> 
> > No. I want all users to get the IANA assigned port unless they specifically
> > override it. For compatibility kernel can't change to the IANA value
> > but user space needs to nudge users in the correct direction.
> 
> You're changing the behavior people get by invoking the tools the way
> they were doing so previously.
> 
> Therefore, just as we can't change the kernel's default, you really
> can't legitimately change iproute2's either.

Ok, then I will make the user specify a port.
We just have to have a way to force new installations to use the correct port.

[PATCH] vxlan: force user to set port value

[Stephen Hemminger]

This change shifts burden onto the users to choose the UDP port value.
There is no default value, the destination port must be specified.

This is a migration compromise. The initial development of VXLAN
used UDP port 5287 but now there is an official assigned port for
VXLAN.  The kernel can't change because of legacy compatibility
but new deployments should not use the legacy port value.

---
 ip/iplink_vxlan.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 2d93ee2..263feca 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	__u8 noage = 0;
 	__u32 age = 0;
 	__u32 maxaddr = 0;
-	__u16 dstport = 4789;
+	__u16 dstport = 0;
+	int dst_port_set = 0;
 	struct ifla_vxlan_port_range range = { 0, 0 };
 
 	while (argc > 0) {
@@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			if (get_u16(&dstport, *argv, 0))
 				invarg("dst port", *argv);
+			dst_port_set = 1;
 		} else if (!matches(*argv, "nolearning")) {
 			learning = 0;
 		} else if (!matches(*argv, "learning")) {
@@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 		}
 		argc--, argv++;
 	}
+
 	if (!vni_set) {
 		fprintf(stderr, "vxlan: missing virtual network identifier\n");
 		return -1;
 	}
+
+	if (!dst_port_set) {
+		fprintf(stderr, "vxlan: destination port not specified\n"
+			"Use 'dstport 4789' to get the IANA assigned value\n");
+		return -1;
+	}
+
 	addattr32(n, 1024, IFLA_VXLAN_ID, vni);
 	if (gaddr)
 		addattr_l(n, 1024, IFLA_VXLAN_GROUP, &gaddr, 4);
@@ -179,6 +189,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 	addattr8(n, 1024, IFLA_VXLAN_RSC, rsc);
 	addattr8(n, 1024, IFLA_VXLAN_L2MISS, l2miss);
 	addattr8(n, 1024, IFLA_VXLAN_L3MISS, l3miss);
+
 	if (noage)
 		addattr32(n, 1024, IFLA_VXLAN_AGEING, 0);
 	else if (age)
-- 
1.7.10.4

Re: [PATCH] vxlan: force user to set port value

[David Miller]

Stephen, this doesn't work either.

You're breaking people's scripts.

Even worse, you're breaking things, and your error message doesn't
even tell the user how to get the previous behavior.  You're telling
them how to get new behavior, which they probably don't give a
crap about.  They want their existing stuff to work.

Stop being in denial, we are stuck with the old port number default.

Again, we cannot change this default without breaking something which
we've already deployed to users.

If this port number issue was so important, we should have done
something about it when we integrated vxlan.  But we didn't, so we
have to live with the consequences.

Now is far too late to change the default.

Re: [PATCH] vxlan: force user to set port value

[Stephen Hemminger]

On Wed, 15 May 2013 14:47:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> 
> Stephen, this doesn't work either.
> 
> You're breaking people's scripts.
> 
> Even worse, you're breaking things, and your error message doesn't
> even tell the user how to get the previous behavior.  You're telling
> them how to get new behavior, which they probably don't give a
> crap about.  They want their existing stuff to work.
> 
> Stop being in denial, we are stuck with the old port number default.
> 
> Again, we cannot change this default without breaking something which
> we've already deployed to users.
> 
> If this port number issue was so important, we should have done
> something about it when we integrated vxlan.  But we didn't, so we
> have to live with the consequences.
> 
> Now is far too late to change the default.

So you want RHEL customers to continue to use the pre-standard Cisco
port in their clouds and be incompatible with standards?

I made a mistake in the initial implementation using that value and every user for time immemorial
has to suffer.

Since every distro patches iproute anyway, let them keep the non-standard
compatibility if that is what they demand.

Re: [PATCH] vxlan: force user to set port value

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Wed, 15 May 2013 15:04:33 -0700

> I made a mistake in the initial implementation using that value and
> every user for time immemorial has to suffer.

We don't break userland, period.

You don't have to explain to me how unfortunate this situation is,
I understand.

But that doesn't give us a license to break things on people.

Re: [PATCH] vxlan: force user to set port value

[Stephen Hemminger]

On Wed, 15 May 2013 14:47:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> 
> Stephen, this doesn't work either.
> 
> You're breaking people's scripts.
> 
> Even worse, you're breaking things, and your error message doesn't
> even tell the user how to get the previous behavior.  You're telling
> them how to get new behavior, which they probably don't give a
> crap about.  They want their existing stuff to work.
> 
> Stop being in denial, we are stuck with the old port number default.
> 
> Again, we cannot change this default without breaking something which
> we've already deployed to users.
> 
> If this port number issue was so important, we should have done
> something about it when we integrated vxlan.  But we didn't, so we
> have to live with the consequences.
> 
> Now is far too late to change the default.


I relented slightly and turned it into a nag, the scripts will still
work but they will be noisy.

Re: [PATCH] vxlan: force user to set port value

[Sridhar Samudrala]

On 5/15/2013 1:48 PM, Stephen Hemminger wrote:
> This change shifts burden onto the users to choose the UDP port value.
> There is no default value, the destination port must be specified.
>
> This is a migration compromise. The initial development of VXLAN
> used UDP port 5287 but now there is an official assigned port for
The original and current kernel default is 8472.
> VXLAN.  The kernel can't change because of legacy compatibility
> but new deployments should not use the legacy port value.
>
> ---
>   ip/iplink_vxlan.c |   13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> index 2d93ee2..263feca 100644
> --- a/ip/iplink_vxlan.c
> +++ b/ip/iplink_vxlan.c
> @@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>   	__u8 noage = 0;
>   	__u32 age = 0;
>   	__u32 maxaddr = 0;
> -	__u16 dstport = 4789;
> +	__u16 dstport = 0;
> +	int dst_port_set = 0;
>   	struct ifla_vxlan_port_range range = { 0, 0 };
>   
>   	while (argc > 0) {
> @@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>   			NEXT_ARG();
>   			if (get_u16(&dstport, *argv, 0))
>   				invarg("dst port", *argv);
> +			dst_port_set = 1;
>   		} else if (!matches(*argv, "nolearning")) {
>   			learning = 0;
>   		} else if (!matches(*argv, "learning")) {
> @@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>   		}
>   		argc--, argv++;
>   	}
> +
>   	if (!vni_set) {
>   		fprintf(stderr, "vxlan: missing virtual network identifier\n");
>   		return -1;
>   	}
> +
> +	if (!dst_port_set) {
> +		fprintf(stderr, "vxlan: destination port not specified\n"
> +			"Use 'dstport 4789' to get the IANA assigned value\n");
> +		return -1;
> +	}
Just setting the dstport 4789 is not enough. The user has to make sure 
that the
vxlan module is loaded with a module  parameter 'udp_port' set to 4789.

Thanks
Sridhar
> +
>   	addattr32(n, 1024, IFLA_VXLAN_ID, vni);
>   	if (gaddr)
>   		addattr_l(n, 1024, IFLA_VXLAN_GROUP, &gaddr, 4);
> @@ -179,6 +189,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>   	addattr8(n, 1024, IFLA_VXLAN_RSC, rsc);
>   	addattr8(n, 1024, IFLA_VXLAN_L2MISS, l2miss);
>   	addattr8(n, 1024, IFLA_VXLAN_L3MISS, l3miss);
> +
>   	if (noage)
>   		addattr32(n, 1024, IFLA_VXLAN_AGEING, 0);
>   	else if (age)

Re: [PATCH] vxlan: force user to set port value

[Stephen Hemminger]

On Wed, 15 May 2013 14:57:54 -0700
Sridhar Samudrala <samudrala.sridhar@gmail.com> wrote:

> On 5/15/2013 1:48 PM, Stephen Hemminger wrote:
> > This change shifts burden onto the users to choose the UDP port value.
> > There is no default value, the destination port must be specified.
> >
> > This is a migration compromise. The initial development of VXLAN
> > used UDP port 5287 but now there is an official assigned port for
> The original and current kernel default is 8472.
> > VXLAN.  The kernel can't change because of legacy compatibility
> > but new deployments should not use the legacy port value.
> >
> > ---
> >   ip/iplink_vxlan.c |   13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> > index 2d93ee2..263feca 100644
> > --- a/ip/iplink_vxlan.c
> > +++ b/ip/iplink_vxlan.c
> > @@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
> >   	__u8 noage = 0;
> >   	__u32 age = 0;
> >   	__u32 maxaddr = 0;
> > -	__u16 dstport = 4789;
> > +	__u16 dstport = 0;
> > +	int dst_port_set = 0;
> >   	struct ifla_vxlan_port_range range = { 0, 0 };
> >   
> >   	while (argc > 0) {
> > @@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
> >   			NEXT_ARG();
> >   			if (get_u16(&dstport, *argv, 0))
> >   				invarg("dst port", *argv);
> > +			dst_port_set = 1;
> >   		} else if (!matches(*argv, "nolearning")) {
> >   			learning = 0;
> >   		} else if (!matches(*argv, "learning")) {
> > @@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
> >   		}
> >   		argc--, argv++;
> >   	}
> > +
> >   	if (!vni_set) {
> >   		fprintf(stderr, "vxlan: missing virtual network identifier\n");
> >   		return -1;
> >   	}
> > +
> > +	if (!dst_port_set) {
> > +		fprintf(stderr, "vxlan: destination port not specified\n"
> > +			"Use 'dstport 4789' to get the IANA assigned value\n");
> > +		return -1;
> > +	}
> Just setting the dstport 4789 is not enough. The user has to make sure 
> that the
> vxlan module is loaded with a module  parameter 'udp_port' set to 4789.

With this (and other fix) the kernel parameter is irrelevant.