vxlan: Liberal parsing & basic VXLAN-gpe
Subsystems (2)
cover vxlan: Liberal parsing & basic VXLAN-gpe
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(-)
Signature · 1 line
1.9.3
PATCH 1/2 vxlan: Be liberal on receive and only require the I bit to be set
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.
--- drivers/net/vxlan.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 10 deletions(-)
drivers/net/vxlan.c +43 -10
@@ -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));
Signature · 1 line
1.9.3
Re: [PATCH 1/2 net-next] vxlan: Be liberal on receive and only require the I bit to be set
drivers/net/vxlan.c static int vxlan6_xmit_skb(struct vxlan_sock *vs, · line 1653
Quoted 10 lines · On Fri, 11 Jul 2014 18:59:49 +0200 Thomas Graf <[email protected]> wrote:
drivers/net/vxlan.c +3
-1@@ -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
Quoted 6 lines · Stephen Hemminger
Quoted 3 lines · On Fri, 11 Jul 2014 18:59:49 +0200 Thomas Graf <[email protected]> wrote:
(quoted diff) +0
-0@@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs, }
Quoted 10 lines
drivers/net/vxlan.c static int vxlan6_xmit_skb(struct vxlan_sock *vs, · line 1653
Quoted 7 lines
drivers/net/vxlan.c +3
-1vxh = (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
In reply to Stephen HemmingerFrom: Stephen Hemminger
Quoted 6 lines
Quoted 3 lines · On Fri, 11 Jul 2014 18:59:49 +0200 Thomas Graf <[email protected]> wrote:
(quoted diff) +0
-0@@ -1617,7 +1646,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs, }
Quoted 10 lines
drivers/net/vxlan.c static int vxlan6_xmit_skb(struct vxlan_sock *vs, · line 1653
Quoted 7 lines
drivers/net/vxlan.c +3
-1vxh = (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
In reply to Thomas GrafQuoted 8 lines · 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.
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
Quoted 99 lines
Cc Pritesh Kothari (pritkoth) <[email protected]>Cc Madhu Challa <[email protected]>Signed-off-by Thomas Graf <[email protected]>--- drivers/net/vxlan.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 10 deletions(-)drivers/net/vxlan.c +43
-11@@ -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
Quoted 9 lines · Tom Herbert
Quoted 8 lines · 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.
Quoted 6 lines
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
Quoted 10 lines · Thomas Graf
Quoted 9 lines · Tom Herbert
Quoted 8 lines · 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.
Quoted 8 lines
Quoted 6 lines
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 vxlan: Minimal support for Generic Protocol Extension (VXLAN-gpe)
In reply to Thomas GrafCheck 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.
--- drivers/net/vxlan.c | 55 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 12 deletions(-)
drivers/net/vxlan.c +43 -12
@@ -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));
Signature · 1 line
1.9.3