Patchwork [net-next,3/6] vxlan: fix byte order issues with NDA_PORT

login
register
mail settings
Submitter stephen hemminger
Date April 27, 2013, 9:31 p.m.
Message ID <20130427213258.279619123@vyatta.com>
Download mbox | patch
Permalink /patch/240205/
State Accepted
Delegated to: David Miller
Headers show

Comments

stephen hemminger - April 27, 2013, 9:31 p.m.
The NDA_PORT attribute was added, but the author wasn't careful
about width (port is 16 bits), or byte order.  The attribute was
being dumped as 16 bits, but only 32 bit value would be accepted
when setting up a device. Also, the remote port is in network
byte order and was being compared with default port in host byte
order.

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




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Stevens - April 29, 2013, 3:47 p.m.
netdev-owner@vger.kernel.org wrote on 04/27/2013 05:31:54 PM:

> From: Stephen Hemminger <stephen@networkplumber.org>

> The NDA_PORT attribute was added, but the author wasn't careful
> about width (port is 16 bits), or byte order.  The attribute was
> being dumped as 16 bits, but only 32 bit value would be accepted
> when setting up a device. Also, the remote port is in network
> byte order and was being compared with default port in host byte
> order.

        They were all in host order until your patch (though it looks like
I may have used __be16 instead of __u32 and it "worked" despite the
type error. I left it as 32 bits for netlink to be int-sized and not
have unintended promotions, not to mention because "vxlan_port" is 32bit,
and still is. I'm ok with switching them to network order and 16 bit,
but I think you should make "vxlan_port" the same byte
order and size as "remote_port" so the comparisons below don't require
byte-swapping at run-time, as the original code did not.

So my suggestion is you make vxlan_port be __be16 as part of this
patch, too.

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> 
> --- a/drivers/net/vxlan.c   2013-04-27 13:38:57.232338603 -0700
> +++ b/drivers/net/vxlan.c   2013-04-27 13:56:54.050412049 -0700
> @@ -192,7 +192,7 @@ static int vxlan_fdb_info(struct sk_buff
>     if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
>        goto nla_put_failure;
> 
> -   if (rdst->remote_port && rdst->remote_port != vxlan_port &&
> +   if (rdst->remote_port && rdst->remote_port != htons(vxlan_port) &&
>         nla_put_be16(skb, NDA_PORT, rdst->remote_port))
>        goto nla_put_failure;
>     if (rdst->remote_vni != vxlan->default_dst.remote_vni &&
> @@ -222,7 +222,7 @@ static inline size_t vxlan_nlmsg_size(vo
>     return NLMSG_ALIGN(sizeof(struct ndmsg))
>        + nla_total_size(ETH_ALEN) /* NDA_LLADDR */
>        + nla_total_size(sizeof(__be32)) /* NDA_DST */
> -      + nla_total_size(sizeof(__be32)) /* NDA_PORT */
> +      + nla_total_size(sizeof(__be16)) /* NDA_PORT */
>        + nla_total_size(sizeof(__be32)) /* NDA_VNI */
>        + nla_total_size(sizeof(__u32)) /* NDA_IFINDEX */
>        + nla_total_size(sizeof(struct nda_cacheinfo));
> @@ -317,7 +317,7 @@ static struct vxlan_fdb *vxlan_find_mac(
> 
>  /* Add/update destinations for multicast */
>  static int vxlan_fdb_append(struct vxlan_fdb *f,
> -             __be32 ip, __u32 port, __u32 vni, __u32 ifindex)
> +             __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
>  {
>     struct vxlan_rdst *rd_prev, *rd;
> 
> @@ -346,7 +346,7 @@ static int vxlan_fdb_append(struct vxlan
>  static int vxlan_fdb_create(struct vxlan_dev *vxlan,
>               const u8 *mac, __be32 ip,
>               __u16 state, __u16 flags,
> -             __u32 port, __u32 vni, __u32 ifindex,
> +             __be16 port, __u32 vni, __u32 ifindex,
>               __u8 ndm_flags)
>  {
>     struct vxlan_fdb *f;
> @@ -444,7 +444,8 @@ static int vxlan_fdb_add(struct ndmsg *n
>     struct vxlan_dev *vxlan = netdev_priv(dev);
>     struct net *net = dev_net(vxlan->dev);
>     __be32 ip;
> -   u32 port, vni, ifindex;
> +   __be16 port;
> +   u32 vni, ifindex;
>     int err;
> 
>     if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_REACHABLE))) {
> @@ -462,11 +463,11 @@ static int vxlan_fdb_add(struct ndmsg *n
>     ip = nla_get_be32(tb[NDA_DST]);
> 
>     if (tb[NDA_PORT]) {
> -      if (nla_len(tb[NDA_PORT]) != sizeof(u32))
> +      if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
>           return -EINVAL;
> -      port = nla_get_u32(tb[NDA_PORT]);
> +      port = nla_get_be16(tb[NDA_PORT]);
>     } else
> -      port = vxlan_port;
> +      port = htons(vxlan_port);
> 
>     if (tb[NDA_VNI]) {
>        if (nla_len(tb[NDA_VNI]) != sizeof(u32))
> @@ -489,8 +490,8 @@ static int vxlan_fdb_add(struct ndmsg *n
>        ifindex = 0;
> 
>     spin_lock_bh(&vxlan->hash_lock);
> -   err = vxlan_fdb_create(vxlan, addr, ip, ndm->ndm_state, flags, port,
> -      vni, ifindex, ndm->ndm_flags);
> +   err = vxlan_fdb_create(vxlan, addr, ip, ndm->ndm_state, flags,
> +                port, vni, ifindex, ndm->ndm_flags);
>     spin_unlock_bh(&vxlan->hash_lock);
> 
>     return err;
> @@ -964,12 +965,13 @@ static netdev_tx_t vxlan_xmit_one(struct
>     struct udphdr *uh;
>     struct flowi4 fl4;
>     __be32 dst;
> -   __u16 src_port, dst_port;
> +   __u16 src_port;
> +   __be16 dst_port;
>          u32 vni;
>     __be16 df = 0;
>     __u8 tos, ttl;
> 
> -   dst_port = rdst->remote_port ? rdst->remote_port : vxlan_port;
> +   dst_port = rdst->remote_port ? rdst->remote_port : 
htons(vxlan_port);
>     vni = rdst->remote_vni;
>     dst = rdst->remote_ip;
> 
> @@ -1050,7 +1052,7 @@ static netdev_tx_t vxlan_xmit_one(struct
>     skb_reset_transport_header(skb);
>     uh = udp_hdr(skb);
> 
> -   uh->dest = htons(dst_port);
> +   uh->dest = dst_port;
>     uh->source = htons(src_port);
> 
>     uh->len = htons(skb->len);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - April 29, 2013, 11:40 p.m.
On Mon, 29 Apr 2013 11:47:26 -0400
David Stevens <dlstevens@us.ibm.com> wrote:

>         They were all in host order until your patch (though it looks like
> I may have used __be16 instead of __u32 and it "worked" despite the
> type error. I left it as 32 bits for netlink to be int-sized and not
> have unintended promotions, not to mention because "vxlan_port" is 32bit,
> and still is. I'm ok with switching them to network order and 16 bit,
> but I think you should make "vxlan_port" the same byte
> order and size as "remote_port" so the comparisons below don't require
> byte-swapping at run-time, as the original code did not.

Module parameters need to be host order, and there isn't really a good
way to type them as 16 bit.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/net/vxlan.c	2013-04-27 13:38:57.232338603 -0700
+++ b/drivers/net/vxlan.c	2013-04-27 13:56:54.050412049 -0700
@@ -192,7 +192,7 @@  static int vxlan_fdb_info(struct sk_buff
 	if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
 		goto nla_put_failure;
 
-	if (rdst->remote_port && rdst->remote_port != vxlan_port &&
+	if (rdst->remote_port && rdst->remote_port != htons(vxlan_port) &&
 	    nla_put_be16(skb, NDA_PORT, rdst->remote_port))
 		goto nla_put_failure;
 	if (rdst->remote_vni != vxlan->default_dst.remote_vni &&
@@ -222,7 +222,7 @@  static inline size_t vxlan_nlmsg_size(vo
 	return NLMSG_ALIGN(sizeof(struct ndmsg))
 		+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
 		+ nla_total_size(sizeof(__be32)) /* NDA_DST */
-		+ nla_total_size(sizeof(__be32)) /* NDA_PORT */
+		+ nla_total_size(sizeof(__be16)) /* NDA_PORT */
 		+ nla_total_size(sizeof(__be32)) /* NDA_VNI */
 		+ nla_total_size(sizeof(__u32)) /* NDA_IFINDEX */
 		+ nla_total_size(sizeof(struct nda_cacheinfo));
@@ -317,7 +317,7 @@  static struct vxlan_fdb *vxlan_find_mac(
 
 /* Add/update destinations for multicast */
 static int vxlan_fdb_append(struct vxlan_fdb *f,
-			    __be32 ip, __u32 port, __u32 vni, __u32 ifindex)
+			    __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
 {
 	struct vxlan_rdst *rd_prev, *rd;
 
@@ -346,7 +346,7 @@  static int vxlan_fdb_append(struct vxlan
 static int vxlan_fdb_create(struct vxlan_dev *vxlan,
 			    const u8 *mac, __be32 ip,
 			    __u16 state, __u16 flags,
-			    __u32 port, __u32 vni, __u32 ifindex,
+			    __be16 port, __u32 vni, __u32 ifindex,
 			    __u8 ndm_flags)
 {
 	struct vxlan_fdb *f;
@@ -444,7 +444,8 @@  static int vxlan_fdb_add(struct ndmsg *n
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct net *net = dev_net(vxlan->dev);
 	__be32 ip;
-	u32 port, vni, ifindex;
+	__be16 port;
+	u32 vni, ifindex;
 	int err;
 
 	if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_REACHABLE))) {
@@ -462,11 +463,11 @@  static int vxlan_fdb_add(struct ndmsg *n
 	ip = nla_get_be32(tb[NDA_DST]);
 
 	if (tb[NDA_PORT]) {
-		if (nla_len(tb[NDA_PORT]) != sizeof(u32))
+		if (nla_len(tb[NDA_PORT]) != sizeof(__be16))
 			return -EINVAL;
-		port = nla_get_u32(tb[NDA_PORT]);
+		port = nla_get_be16(tb[NDA_PORT]);
 	} else
-		port = vxlan_port;
+		port = htons(vxlan_port);
 
 	if (tb[NDA_VNI]) {
 		if (nla_len(tb[NDA_VNI]) != sizeof(u32))
@@ -489,8 +490,8 @@  static int vxlan_fdb_add(struct ndmsg *n
 		ifindex = 0;
 
 	spin_lock_bh(&vxlan->hash_lock);
-	err = vxlan_fdb_create(vxlan, addr, ip, ndm->ndm_state, flags, port,
-		vni, ifindex, ndm->ndm_flags);
+	err = vxlan_fdb_create(vxlan, addr, ip, ndm->ndm_state, flags,
+			       port, vni, ifindex, ndm->ndm_flags);
 	spin_unlock_bh(&vxlan->hash_lock);
 
 	return err;
@@ -964,12 +965,13 @@  static netdev_tx_t vxlan_xmit_one(struct
 	struct udphdr *uh;
 	struct flowi4 fl4;
 	__be32 dst;
-	__u16 src_port, dst_port;
+	__u16 src_port;
+	__be16 dst_port;
         u32 vni;
 	__be16 df = 0;
 	__u8 tos, ttl;
 
-	dst_port = rdst->remote_port ? rdst->remote_port : vxlan_port;
+	dst_port = rdst->remote_port ? rdst->remote_port : htons(vxlan_port);
 	vni = rdst->remote_vni;
 	dst = rdst->remote_ip;
 
@@ -1050,7 +1052,7 @@  static netdev_tx_t vxlan_xmit_one(struct
 	skb_reset_transport_header(skb);
 	uh = udp_hdr(skb);
 
-	uh->dest = htons(dst_port);
+	uh->dest = dst_port;
 	uh->source = htons(src_port);
 
 	uh->len = htons(skb->len);