[PATCH 0/2 net-next] vxlan: Liberal parsing & basic VXLAN-gpe

[PATCH 0/2 net-next] vxlan: Liberal parsing & basic VXLAN-gpe

[Thomas Graf]

The following series makes VXLAN receive more liberal in what it accepts.
The receive behaviour is now in accordance with the draft. It also
establishes basic support for VXLAN-gpe to allow carrying non ethernet
in the future.

Thomas Graf (2):
  vxlan: Be liberal on receive and only require the I bit to be set
  vxlan: Minimal support for Generic Protocol Extension (VXLAN-gpe)

 drivers/net/vxlan.c | 88 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 12 deletions(-)

-- 
1.9.3

[PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Thomas Graf]

The VXLAN receive code is currently conservative in what it accepts and
will reject any frame that uses any of the reserved fields. The VXLAN
draft specifies that "reserved fields MUST be set to zero on transmit
and ignored on receive." though.

Be liberal in only requiring the I bit to allow for VXLAN extensions
to be implemented.

Cc: Pritesh Kothari (pritkoth) <pritkoth@cisco.com>
Cc: Madhu Challa <challa@noironetworks.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 drivers/net/vxlan.c | 53 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index e6808f7..2e9fbcc 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -63,12 +63,43 @@
 #define VXLAN_VID_MASK	(VXLAN_N_VID - 1)
 #define VXLAN_HLEN (sizeof(struct udphdr) + sizeof(struct vxlanhdr))
 
-#define VXLAN_FLAGS 0x08000000	/* struct vxlanhdr.vx_flags required value. */
-
+/* Base VXLAN header:
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |R|R|R|R|I|R|R|R|               Reserved                        |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                VXLAN Network Identifier (VNI) |   Reserved    |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * I = 1	VXLAN Network Identifier (VNI) present
+ */
 /* VXLAN protocol header */
 struct vxlanhdr {
-	__be32 vx_flags;
-	__be32 vx_vni;
+#ifdef __LITTLE_ENDIAN_BITFIELD
+	__u8	reserved_flags1:3,
+		vni_present:1,
+		reserved_flags2:4;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	__u8	reserved_flags2:4,
+		vni_present:1,
+		reserved_flags1:3;
+#else
+#error	"Please fix <asm/byteorder.h>"
+#endif
+	__u8	vx_reserved1;
+	__be16	vx_reserved2;
+	__be32	vx_vni;
+};
+
+union vxlan_oct_hdr {
+	struct vxlanhdr	hdr;
+	__u8		octets[8];
+};
+
+#define vxlan_flags(vxh) (((union vxlan_oct_hdr *)(vxh))->octets[0])
+
+enum {
+	VXLAN_FLAG_VNI		= 0x08,
+	VXLAN_FLAG_RESERVED	= 0xF7,
 };
 
 /* UDP port for VXLAN traffic.
@@ -1141,12 +1172,10 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (!pskb_may_pull(skb, VXLAN_HLEN))
 		goto error;
 
-	/* Return packets with reserved bits set */
 	vxh = (struct vxlanhdr *)(udp_hdr(skb) + 1);
-	if (vxh->vx_flags != htonl(VXLAN_FLAGS) ||
-	    (vxh->vx_vni & htonl(0xff))) {
+	if (!vxh->vni_present) {
 		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
-			   ntohl(vxh->vx_flags), ntohl(vxh->vx_vni));
+			   vxlan_flags(vxh), ntohl(vxh->vx_vni));
 		goto error;
 	}
 
@@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
 	}
 
 	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
-	vxh->vx_flags = htonl(VXLAN_FLAGS);
+	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
+	vxh->vx_reserved1 = 0;
+	vxh->vx_reserved2 = 0;
 	vxh->vx_vni = vni;
 
 	__skb_push(skb, sizeof(*uh));
@@ -1689,7 +1720,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
 	}
 
 	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
-	vxh->vx_flags = htonl(VXLAN_FLAGS);
+	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
+	vxh->vx_reserved1 = 0;
+	vxh->vx_reserved2 = 0;
 	vxh->vx_vni = vni;
 
 	__skb_push(skb, sizeof(*uh));
-- 
1.9.3

Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Stephen Hemminger]

On Fri, 11 Jul 2014 18:59:49 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> @@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
>  	}
>  
>  	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> -	vxh->vx_flags = htonl(VXLAN_FLAGS);
> +	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
> +	vxh->vx_reserved1 = 0;
> +	vxh->vx_reserved2 = 0;
>  	vxh->vx_vni = vni;
>  
 
Okay, but initializing bitfields generates crappy code.
Can we just alias and do one assignment.

Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Thomas Graf]

On 07/11/14 at 12:41pm, Stephen Hemminger wrote:
> On Fri, 11 Jul 2014 18:59:49 +0200
> Thomas Graf <tgraf@suug.ch> wrote:
> 
> > @@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
> >  	}
> >  
> >  	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> > -	vxh->vx_flags = htonl(VXLAN_FLAGS);
> > +	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
> > +	vxh->vx_reserved1 = 0;
> > +	vxh->vx_reserved2 = 0;
> >  	vxh->vx_vni = vni;
> >  
>  
> Okay, but initializing bitfields generates crappy code.
> Can we just alias and do one assignment.

Sure, I'll resend.

RE: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[David Laight]

From: Stephen Hemminger
> On Fri, 11 Jul 2014 18:59:49 +0200
> Thomas Graf <tgraf@suug.ch> wrote:
> 
> > @@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
> >  	}
> >
> >  	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> > -	vxh->vx_flags = htonl(VXLAN_FLAGS);
> > +	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
> > +	vxh->vx_reserved1 = 0;
> > +	vxh->vx_reserved2 = 0;
> >  	vxh->vx_vni = vni;
> >
> 
> Okay, but initializing bitfields generates crappy code.
> Can we just alias and do one assignment.

If you write to all the bitfields gcc ought to manage to do a single write.
If it doesn't then all the bitfields should be ripped out :-)

	David

Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Tom Herbert]

On Fri, Jul 11, 2014 at 9:59 AM, Thomas Graf <tgraf@suug.ch> wrote:
> The VXLAN receive code is currently conservative in what it accepts and
> will reject any frame that uses any of the reserved fields. The VXLAN
> draft specifies that "reserved fields MUST be set to zero on transmit
> and ignored on receive." though.
>
> Be liberal in only requiring the I bit to allow for VXLAN extensions
> to be implemented.
>
This is not robust (this is a problem in the VXLAN spec not your
patch). There is no requirement that the VXLAN bits are optional. For
example, if a receiver accepts a GPE packet but doesn't implement it
the packet will be misinterpreted. I've already pointed this out to
the VLXAN folks on nvo3 list. Dropping packets with unknown bits set
is the only sane approach.

Tom


> Cc: Pritesh Kothari (pritkoth) <pritkoth@cisco.com>
> Cc: Madhu Challa <challa@noironetworks.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  drivers/net/vxlan.c | 53 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index e6808f7..2e9fbcc 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -63,12 +63,43 @@
>  #define VXLAN_VID_MASK (VXLAN_N_VID - 1)
>  #define VXLAN_HLEN (sizeof(struct udphdr) + sizeof(struct vxlanhdr))
>
> -#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */
> -
> +/* Base VXLAN header:
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * |R|R|R|R|I|R|R|R|               Reserved                        |
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * |                VXLAN Network Identifier (VNI) |   Reserved    |
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + *
> + * I = 1       VXLAN Network Identifier (VNI) present
> + */
>  /* VXLAN protocol header */
>  struct vxlanhdr {
> -       __be32 vx_flags;
> -       __be32 vx_vni;
> +#ifdef __LITTLE_ENDIAN_BITFIELD
> +       __u8    reserved_flags1:3,
> +               vni_present:1,
> +               reserved_flags2:4;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +       __u8    reserved_flags2:4,
> +               vni_present:1,
> +               reserved_flags1:3;
> +#else
> +#error "Please fix <asm/byteorder.h>"
> +#endif
> +       __u8    vx_reserved1;
> +       __be16  vx_reserved2;
> +       __be32  vx_vni;
> +};
> +
> +union vxlan_oct_hdr {
> +       struct vxlanhdr hdr;
> +       __u8            octets[8];
> +};
> +
> +#define vxlan_flags(vxh) (((union vxlan_oct_hdr *)(vxh))->octets[0])
> +
> +enum {
> +       VXLAN_FLAG_VNI          = 0x08,
> +       VXLAN_FLAG_RESERVED     = 0xF7,
>  };
>
>  /* UDP port for VXLAN traffic.
> @@ -1141,12 +1172,10 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>         if (!pskb_may_pull(skb, VXLAN_HLEN))
>                 goto error;
>
> -       /* Return packets with reserved bits set */
>         vxh = (struct vxlanhdr *)(udp_hdr(skb) + 1);
> -       if (vxh->vx_flags != htonl(VXLAN_FLAGS) ||
> -           (vxh->vx_vni & htonl(0xff))) {
> +       if (!vxh->vni_present) {
>                 netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
> -                          ntohl(vxh->vx_flags), ntohl(vxh->vx_vni));
> +                          vxlan_flags(vxh), ntohl(vxh->vx_vni));
>                 goto error;
>         }
>
> @@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
>         }
>
>         vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> -       vxh->vx_flags = htonl(VXLAN_FLAGS);
> +       vxlan_flags(vxh) = VXLAN_FLAG_VNI;
> +       vxh->vx_reserved1 = 0;
> +       vxh->vx_reserved2 = 0;
>         vxh->vx_vni = vni;
>
>         __skb_push(skb, sizeof(*uh));
> @@ -1689,7 +1720,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
>         }
>
>         vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> -       vxh->vx_flags = htonl(VXLAN_FLAGS);
> +       vxlan_flags(vxh) = VXLAN_FLAG_VNI;
> +       vxh->vx_reserved1 = 0;
> +       vxh->vx_reserved2 = 0;
>         vxh->vx_vni = vni;
>
>         __skb_push(skb, sizeof(*uh));
> --
> 1.9.3
>

Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Thomas Graf]

On 07/13/14 at 03:08pm, Tom Herbert wrote:
> On Fri, Jul 11, 2014 at 9:59 AM, Thomas Graf <tgraf@suug.ch> wrote:
> > The VXLAN receive code is currently conservative in what it accepts and
> > will reject any frame that uses any of the reserved fields. The VXLAN
> > draft specifies that "reserved fields MUST be set to zero on transmit
> > and ignored on receive." though.
> >
> > Be liberal in only requiring the I bit to allow for VXLAN extensions
> > to be implemented.
> >
> This is not robust (this is a problem in the VXLAN spec not your
> patch). There is no requirement that the VXLAN bits are optional. For
> example, if a receiver accepts a GPE packet but doesn't implement it
> the packet will be misinterpreted. I've already pointed this out to
> the VLXAN folks on nvo3 list. Dropping packets with unknown bits set
> is the only sane approach.

I agree, it's a mess.

At least VXLAN-gpe has realized this and will reserve an individual
port number. The port number is not yet fixed so it can't be enforced
yet but once that is done, gpe frame acceptance can be bound to the use
of that particular UDP port.

Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set

[Nicolas Dichtel]

Le 14/07/2014 10:18, Thomas Graf a écrit :
> On 07/13/14 at 03:08pm, Tom Herbert wrote:
>> On Fri, Jul 11, 2014 at 9:59 AM, Thomas Graf <tgraf@suug.ch> wrote:
>>> The VXLAN receive code is currently conservative in what it accepts and
>>> will reject any frame that uses any of the reserved fields. The VXLAN
>>> draft specifies that "reserved fields MUST be set to zero on transmit
>>> and ignored on receive." though.
>>>
>>> Be liberal in only requiring the I bit to allow for VXLAN extensions
>>> to be implemented.
>>>
>> This is not robust (this is a problem in the VXLAN spec not your
>> patch). There is no requirement that the VXLAN bits are optional. For
>> example, if a receiver accepts a GPE packet but doesn't implement it
>> the packet will be misinterpreted. I've already pointed this out to
>> the VLXAN folks on nvo3 list. Dropping packets with unknown bits set
>> is the only sane approach.
>
> I agree, it's a mess.
+1

[PATCH 2/2 net-next] vxlan: Minimal support for Generic Protocol Extension (VXLAN-gpe)

[Thomas Graf]

Check for the presence of the GPE bit and interpret the inner protocol
field accordingly. For now, only ethernet in VXLAN is supported. Other
protocol types will result in the packet being dropped.

Based on original work by Pritesh Kothari.

Cc: Madhu Challa <challa@noironetworks.com>
Co-authored-by: Pritesh Kothari (pritkoth) <pritesh.kothari@cisco.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 drivers/net/vxlan.c | 55 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 2e9fbcc..c9af24b 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -65,28 +65,39 @@
 
 /* Base VXLAN header:
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * |R|R|R|R|I|R|R|R|               Reserved                        |
+ * |R|R|R|R|I|P|R|R|               Reserved                        |
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  * |                VXLAN Network Identifier (VNI) |   Reserved    |
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  *
  * I = 1	VXLAN Network Identifier (VNI) present
+ * P = 1	Generic Protocol Extension (VXLAN-gpe)
+ *
+ * VXLAN-gpe:
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |R|R|R|R|I|1|R|R|   Reserved    |     Inner Protocol Type       |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                VXLAN Network Identifier (VNI) |   Reserved    |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  */
+
 /* VXLAN protocol header */
 struct vxlanhdr {
 #ifdef __LITTLE_ENDIAN_BITFIELD
-	__u8	reserved_flags1:3,
+	__u8	reserved_flags1:2,
+		gpe:1,
 		vni_present:1,
 		reserved_flags2:4;
 #elif defined(__BIG_ENDIAN_BITFIELD)
 	__u8	reserved_flags2:4,
 		vni_present:1,
-		reserved_flags1:3;
+		gpe:1,
+		reserved_flags1:2;
 #else
 #error	"Please fix <asm/byteorder.h>"
 #endif
 	__u8	vx_reserved1;
-	__be16	vx_reserved2;
+	__be16	vx_inner_proto; /* Only valid if gpe flag is set */
 	__be32	vx_vni;
 };
 
@@ -98,8 +109,9 @@ union vxlan_oct_hdr {
 #define vxlan_flags(vxh) (((union vxlan_oct_hdr *)(vxh))->octets[0])
 
 enum {
+	VXLAN_FLAG_GPE		= 0x04,
 	VXLAN_FLAG_VNI		= 0x08,
-	VXLAN_FLAG_RESERVED	= 0xF7,
+	VXLAN_FLAG_RESERVED	= 0xF3,
 };
 
 /* UDP port for VXLAN traffic.
@@ -1162,6 +1174,26 @@ static void vxlan_igmp_leave(struct work_struct *work)
 	dev_put(vxlan->dev);
 }
 
+static int vxlan_handle_extensions(struct sk_buff *skb, struct vxlanhdr *vxh)
+{
+	if (!vxh->vni_present) {
+		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
+			   vxlan_flags(vxh), ntohl(vxh->vx_vni));
+		return -EINVAL;
+	}
+
+	if (vxh->gpe) {
+		/* Only Ethernet in VXLAN supported for now */
+		if (vxh->vx_inner_proto != htons(ETH_P_TEB)) {
+			netdev_dbg(skb->dev, "unsupported inner vxlan proto %#x vni %#x\n",
+				   ntohs(vxh->vx_inner_proto), ntohl(vxh->vx_vni));
+			return -EPFNOSUPPORT;
+		}
+	}
+
+	return 0;
+}
+
 /* Callback from net/ipv4/udp.c to receive packets */
 static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
@@ -1173,11 +1205,10 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		goto error;
 
 	vxh = (struct vxlanhdr *)(udp_hdr(skb) + 1);
-	if (!vxh->vni_present) {
-		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
-			   vxlan_flags(vxh), ntohl(vxh->vx_vni));
-		goto error;
-	}
+
+	if (unlikely(vxlan_flags(vxh) != VXLAN_FLAG_VNI))
+		if (vxlan_handle_extensions(skb, vxh) < 0)
+			goto error;
 
 	if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
 		goto drop;
@@ -1648,7 +1679,7 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
 	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
 	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
 	vxh->vx_reserved1 = 0;
-	vxh->vx_reserved2 = 0;
+	vxh->vx_inner_proto = 0;
 	vxh->vx_vni = vni;
 
 	__skb_push(skb, sizeof(*uh));
@@ -1722,7 +1753,7 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
 	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
 	vxlan_flags(vxh) = VXLAN_FLAG_VNI;
 	vxh->vx_reserved1 = 0;
-	vxh->vx_reserved2 = 0;
+	vxh->vx_inner_proto = 0;
 	vxh->vx_vni = vni;
 
 	__skb_push(skb, sizeof(*uh));
-- 
1.9.3