netdev - Re: [Patch net-next v4 4/5] vxlan: add scope

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OFA1761298.DF396ECD-ON85257B52.003AE2CA-85257B52.003DBDAB@us.ibm.com>
Date:	Fri, 19 Apr 2013 07:14:21 -0400
From:	David Stevens <dlstevens@...ibm.com>
To:	Cong Wang <amwang@...hat.com>
Cc:	Cong Wang <amwang@...hat.com>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [Patch net-next v4 4/5] vxlan: add scope_id support for ll addr

Cong Wang <amwang@...hat.com> wrote on 04/17/2013 01:10:21 AM:
 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 43ed40f..531c5e2 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -92,9 +92,10 @@ struct vxlan_addr {
>        struct sockaddr_in6   sin6;
>        struct sockaddr      sa;
>     } u;
> -#define va_sin  u.sin.sin_addr.s_addr
> -#define va_sin6 u.sin6.sin6_addr
> -#define va_sa   u.sa.sa_family
> +#define va_sin      u.sin.sin_addr.s_addr
> +#define va_sin6      u.sin6.sin6_addr
> +#define va_scope_id   u.sin6.sin6_scope_id
> +#define va_sa      u.sa.sa_family
>  };

        As I commented before, you're obscuring the types here,
which makes it less readable. "va_sin6" ought to be a sockaddr_in6,
not a sin6_addr, and the scope id ought to then be 
"va_sin6.sin6_scope_id",
without any other #define necessary for it.



> @@ -1708,6 +1712,17 @@ static int vxlan_newlink(struct net *net, 
> struct net_device *dev,
>        /* update header length based on lower device */
>        dev->hard_header_len = lowerdev->hard_header_len +
>                     VXLAN_HEADROOM;
> +#if IS_ENABLED(CONFIG_IPV6)
> +      dst->remote_ip.va_scope_id = ipv6_iface_scope_id
> (&dst->remote_ip.va_sin6,
> +                         dst->remote_ifindex);
> +      if (ipv6_addr_type(&dst->remote_ip.va_sin6) & 
IPV6_ADDR_LINKLOCAL) {
> +         struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> +         struct sock *sk = vn->sock->sk;
> +
> +                        sk->sk_bound_dev_if = 
dst->remote_ip.va_scope_id;
> +      }
> +#endif
> +
>     }

        The socket is not used for transmits, which is where the 
sin6_scope_id is
relevant-- we need this in vxlan_xmit_one() for the route lookup. It also
permanently binds to an interface, which I believe prevents VXLAN traffic 
from
other interfaces on receive, and this socket is an INADDR_ANY-bound socket 
for
all VXLAN devices on the host.
        So, we shouldn't be doing anything with sk_bound_dev_if, but LL 
addrs need
to use the the sin6_scope_id in the route lookup for transmissions. It 
should
be saved in remote_ifindex per destination and stored in flowi6_oif for 
route
lookup, as the v4 code does now for fdb entries, and LL v6 destinations 
for fdb
entries ought to be done the same way.

        So, no, I don't think this patch is correct or complete.

 +-DLS

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