Running VXLANs over IPv6 link-local addresses allows to use them as a
drop-in replacement for VLANs, avoiding to allocate additional outer IP
addresses to run the VXLAN over.
The first patch is basically a bugfix, not allowing to use link-local
addresses without specifying an interface; it doesn't seem important enough
for net/stable though (and without the second patch, allowing to specify
link-local addresses at all does not result in a working configuration
anyways). The second patch then actually makes VXLAN over link-local IPv6
work by passing interface indices at the right places.
The final patch lifts the limitation of not allowing multiple VXLANs with
the same VNI and port, as long as link-local IPv6 addresses are used and
different interfaces are specified. Again, this brings VXLAN a bit closer
to VLANs feature-wise.
Matthias Schiffer (3):
vxlan: don't allow link-local IPv6 local/remote addresses without
interface
vxlan: fix snooping for link-local IPv6 addresses
vxlan: allow multiple VXLANs with same VNI for IPv6 link-local
addresses
drivers/net/vxlan.c | 120 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 87 insertions(+), 33 deletions(-)
--
2.12.0
If VXLAN is run over link-local IPv6 addresses, it is necessary to store
the ifindex in the FDB entries. Otherwise, the used interface is undefined
and unicast communication will most likely fail.
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/net/vxlan.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index cc0ace73d02e..4c0ef8bbad71 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -941,16 +941,24 @@ static int vxlan_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
*/
static bool vxlan_snoop(struct net_device *dev,
union vxlan_addr *src_ip, const u8 *src_mac,
- __be32 vni)
+ __u32 src_ifindex, __be32 vni)
{
struct vxlan_dev *vxlan = netdev_priv(dev);
struct vxlan_fdb *f;
+ __u32 ifindex = 0;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ if (src_ip->sa.sa_family == AF_INET6 &&
+ (ipv6_addr_type(&src_ip->sin6.sin6_addr) & IPV6_ADDR_LINKLOCAL))
+ ifindex = src_ifindex;
+#endif
f = vxlan_find_mac(vxlan, src_mac, vni);
if (likely(f)) {
struct vxlan_rdst *rdst = first_remote_rcu(f);
- if (likely(vxlan_addr_equal(&rdst->remote_ip, src_ip)))
+ if (likely(vxlan_addr_equal(&rdst->remote_ip, src_ip) &&
+ rdst->remote_ifindex == ifindex))
return false;
/* Don't migrate static entries, drop packets */
@@ -977,7 +985,7 @@ static bool vxlan_snoop(struct net_device *dev,
vxlan->cfg.dst_port,
vni,
vxlan->default_dst.remote_vni,
- 0, NTF_SELF);
+ ifindex, NTF_SELF);
spin_unlock(&vxlan->hash_lock);
}
@@ -1246,6 +1254,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
struct sk_buff *skb, __be32 vni)
{
union vxlan_addr saddr;
+ __u32 ifindex = skb->dev->ifindex;
skb_reset_mac_header(skb);
skb->protocol = eth_type_trans(skb, vxlan->dev);
@@ -1267,7 +1276,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
}
if ((vxlan->flags & VXLAN_F_LEARN) &&
- vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, vni))
+ vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
return false;
return true;
@@ -1974,7 +1983,8 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan,
}
if (dst_vxlan->flags & VXLAN_F_LEARN)
- vxlan_snoop(skb->dev, &loopback, eth_hdr(skb)->h_source, vni);
+ vxlan_snoop(skb->dev, &loopback, eth_hdr(skb)->h_source, 0,
+ vni);
u64_stats_update_begin(&tx_stats->syncp);
tx_stats->tx_packets++;
--
2.12.0
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/net/vxlan.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index e375560cc74e..cc0ace73d02e 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2922,6 +2922,18 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
pr_info("multicast destination requires interface to be specified\n");
return -EINVAL;
}
+#if IS_ENABLED(CONFIG_IPV6)
+ else if (!conf->remote_ifindex &&
+ ((conf->saddr.sa.sa_family == AF_INET6 &&
+ (ipv6_addr_type(&conf->saddr.sin6.sin6_addr) &
+ IPV6_ADDR_LINKLOCAL)) ||
+ (dst->remote_ip.sa.sa_family == AF_INET6 &&
+ (ipv6_addr_type(&dst->remote_ip.sin6.sin6_addr) &
+ IPV6_ADDR_LINKLOCAL)))) {
+ pr_info("link-local local/remote addresses require interface to be specified\n");
+ return -EINVAL;
+ }
+#endif
if (conf->mtu) {
int max_mtu = ETH_MAX_MTU;
--
2.12.0
As link-local addresses are only valid for a single interface, we can allow
to use the same VNI for multiple independent VXLANs, as long as the used
interfaces are distinct. This way, VXLANs can always be used as a drop-in
replacement for VLANs with greater ID space.
This also extends VNI lookup to respect the ifindex when link-local IPv6
addresses are used, so using the same VNI on multiple interfaces can
actually work.
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/net/vxlan.c | 88 ++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 60 insertions(+), 28 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 4c0ef8bbad71..9cd6f6b92cf4 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -224,7 +224,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
return NULL;
}
-static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, __be32 vni)
+static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, int ifindex,
+ __be32 vni)
{
struct vxlan_dev *vxlan;
@@ -233,17 +234,30 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, __be32 vni)
vni = 0;
hlist_for_each_entry_rcu(vxlan, vni_head(vs, vni), hlist) {
- if (vxlan->default_dst.remote_vni == vni)
- return vxlan;
+ if (vxlan->default_dst.remote_vni != vni)
+ continue;
+
+ if (IS_ENABLED(CONFIG_IPV6)) {
+ const struct vxlan_config *cfg = &vxlan->cfg;
+
+ if (cfg->remote_ifindex != 0 &&
+ cfg->remote_ifindex != ifindex &&
+ cfg->saddr.sa.sa_family == AF_INET6 &&
+ (ipv6_addr_type(&cfg->saddr.sin6.sin6_addr) &
+ IPV6_ADDR_LINKLOCAL))
+ continue;
+ }
+
+ return vxlan;
}
return NULL;
}
/* Look up VNI in a per net namespace table */
-static struct vxlan_dev *vxlan_find_vni(struct net *net, __be32 vni,
- sa_family_t family, __be16 port,
- u32 flags)
+static struct vxlan_dev *vxlan_find_vni(struct net *net, int ifindex,
+ __be32 vni, sa_family_t family,
+ __be16 port, u32 flags)
{
struct vxlan_sock *vs;
@@ -251,7 +265,7 @@ static struct vxlan_dev *vxlan_find_vni(struct net *net, __be32 vni,
if (!vs)
return NULL;
- return vxlan_vs_find_vni(vs, vni);
+ return vxlan_vs_find_vni(vs, ifindex, vni);
}
/* Fill in neighbour message in skbuff. */
@@ -1342,7 +1356,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
- vxlan = vxlan_vs_find_vni(vs, vni);
+ vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni);
if (!vxlan)
goto drop;
@@ -2002,8 +2016,10 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan,
}
static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
- struct vxlan_dev *vxlan, union vxlan_addr *daddr,
- __be16 dst_port, __be32 vni, struct dst_entry *dst,
+ struct vxlan_dev *vxlan,
+ union vxlan_addr *daddr,
+ __be16 dst_port, int dst_ifindex, __be32 vni,
+ struct dst_entry *dst,
u32 rt_flags)
{
#if IS_ENABLED(CONFIG_IPV6)
@@ -2019,7 +2035,7 @@ static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
struct vxlan_dev *dst_vxlan;
dst_release(dst);
- dst_vxlan = vxlan_find_vni(vxlan->net, vni,
+ dst_vxlan = vxlan_find_vni(vxlan->net, dst_ifindex, vni,
daddr->sa.sa_family, dst_port,
vxlan->flags);
if (!dst_vxlan) {
@@ -2051,6 +2067,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
struct dst_entry *ndst = NULL;
__be32 vni, label;
__u8 tos, ttl;
+ int ifindex;
int err;
u32 flags = vxlan->flags;
bool udp_sum = false;
@@ -2071,6 +2088,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
dst_port = rdst->remote_port ? rdst->remote_port : vxlan->cfg.dst_port;
vni = (rdst->remote_vni) ? : default_vni;
+ ifindex = rdst->remote_ifindex;
local_ip = vxlan->cfg.saddr;
dst_cache = &rdst->dst_cache;
md->gbp = skb->mark;
@@ -2104,6 +2122,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
dst = &remote_ip;
dst_port = info->key.tp_dst ? : vxlan->cfg.dst_port;
vni = tunnel_id_to_key32(info->key.tun_id);
+ ifindex = 0;
dst_cache = &info->dst_cache;
if (info->options_len)
md = ip_tunnel_info_opts(info);
@@ -2121,8 +2140,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
struct rtable *rt;
__be16 df = 0;
- rt = vxlan_get_route(vxlan, dev, sock4, skb,
- rdst ? rdst->remote_ifindex : 0, tos,
+ rt = vxlan_get_route(vxlan, dev, sock4, skb, ifindex, tos,
dst->sin.sin_addr.s_addr,
&local_ip.sin.sin_addr.s_addr,
dst_port, src_port,
@@ -2135,8 +2153,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
/* Bypass encapsulation if the destination is local */
if (!info) {
err = encap_bypass_if_local(skb, dev, vxlan, dst,
- dst_port, vni, &rt->dst,
- rt->rt_flags);
+ dst_port, ifindex, vni,
+ &rt->dst, rt->rt_flags);
if (err)
goto out_unlock;
} else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT) {
@@ -2158,8 +2176,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
} else {
struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
- ndst = vxlan6_get_route(vxlan, dev, sock6, skb,
- rdst ? rdst->remote_ifindex : 0, tos,
+ ndst = vxlan6_get_route(vxlan, dev, sock6, skb, ifindex, tos,
label, &dst->sin6.sin6_addr,
&local_ip.sin6.sin6_addr,
dst_port, src_port,
@@ -2174,8 +2191,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
u32 rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
err = encap_bypass_if_local(skb, dev, vxlan, dst,
- dst_port, vni, ndst,
- rt6i_flags);
+ dst_port, ifindex, vni,
+ ndst, rt6i_flags);
if (err)
goto out_unlock;
}
@@ -2984,15 +3001,30 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
return 0;
list_for_each_entry(tmp, &vn->vxlan_list, next) {
- if (tmp->cfg.vni == conf->vni &&
- (tmp->default_dst.remote_ip.sa.sa_family == AF_INET6 ||
- tmp->cfg.saddr.sa.sa_family == AF_INET6) == use_ipv6 &&
- tmp->cfg.dst_port == vxlan->cfg.dst_port &&
- (tmp->flags & VXLAN_F_RCV_FLAGS) ==
- (vxlan->flags & VXLAN_F_RCV_FLAGS)) {
- pr_info("duplicate VNI %u\n", be32_to_cpu(conf->vni));
- return -EEXIST;
- }
+ if (tmp->cfg.vni != conf->vni)
+ continue;
+ if ((tmp->default_dst.remote_ip.sa.sa_family == AF_INET6 ||
+ tmp->cfg.saddr.sa.sa_family == AF_INET6) != use_ipv6)
+ continue;
+ if (tmp->cfg.dst_port != vxlan->cfg.dst_port)
+ continue;
+ if ((tmp->flags & VXLAN_F_RCV_FLAGS) !=
+ (vxlan->flags & VXLAN_F_RCV_FLAGS))
+ continue;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ if (conf->remote_ifindex != tmp->cfg.remote_ifindex &&
+ conf->saddr.sa.sa_family == AF_INET6 &&
+ tmp->cfg.saddr.sa.sa_family == AF_INET6 &&
+ (ipv6_addr_type(&conf->saddr.sin6.sin6_addr) &
+ IPV6_ADDR_LINKLOCAL) &&
+ (ipv6_addr_type(&tmp->cfg.saddr.sin6.sin6_addr) &
+ IPV6_ADDR_LINKLOCAL))
+ continue;
+#endif
+
+ pr_info("duplicate VNI %u\n", be32_to_cpu(conf->vni));
+ return -EEXIST;
}
return 0;
--
2.12.0
On Fri, 10 Mar 2017 23:39:42 +0100, Matthias Schiffer wrote:
> Signed-off-by: Matthias Schiffer <[email protected]>
Acked-by: Jiri Benc <[email protected]>
On Fri, 10 Mar 2017 23:39:43 +0100, Matthias Schiffer wrote:
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -941,16 +941,24 @@ static int vxlan_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
> */
> static bool vxlan_snoop(struct net_device *dev,
> union vxlan_addr *src_ip, const u8 *src_mac,
> - __be32 vni)
> + __u32 src_ifindex, __be32 vni)
Just "u32", please. No double underscores for u32 inside the
kernel-only code. (Also in other places in this patch.)
Thanks,
Jiri
On Fri, 10 Mar 2017 23:39:44 +0100, Matthias Schiffer wrote:
> @@ -233,17 +234,30 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, __be32 vni)
> vni = 0;
>
> hlist_for_each_entry_rcu(vxlan, vni_head(vs, vni), hlist) {
> - if (vxlan->default_dst.remote_vni == vni)
> - return vxlan;
> + if (vxlan->default_dst.remote_vni != vni)
> + continue;
> +
> + if (IS_ENABLED(CONFIG_IPV6)) {
> + const struct vxlan_config *cfg = &vxlan->cfg;
> +
> + if (cfg->remote_ifindex != 0 &&
> + cfg->remote_ifindex != ifindex &&
> + cfg->saddr.sa.sa_family == AF_INET6 &&
> + (ipv6_addr_type(&cfg->saddr.sin6.sin6_addr) &
> + IPV6_ADDR_LINKLOCAL))
Calculating this (especially ipv6_addr_type) on every received packet
looks unnecessarily expensive. Just store the fact the the local
address is link-local in a flag during config. And compare the flag
first before considering remote_ifindex.
This is especially important for lwtunnels which can have anything in
the saddr and remote_ifindex, yet those fields are ignored and the
vni 0 entry has to be returned. It also means that the link-local flag
must not be set for lwtunnels.
> +#if IS_ENABLED(CONFIG_IPV6)
> + if (conf->remote_ifindex != tmp->cfg.remote_ifindex &&
> + conf->saddr.sa.sa_family == AF_INET6 &&
> + tmp->cfg.saddr.sa.sa_family == AF_INET6 &&
> + (ipv6_addr_type(&conf->saddr.sin6.sin6_addr) &
> + IPV6_ADDR_LINKLOCAL) &&
> + (ipv6_addr_type(&tmp->cfg.saddr.sin6.sin6_addr) &
> + IPV6_ADDR_LINKLOCAL))
> + continue;
> +#endif
In patch 1, you're checking for either source and destination address
being link-local, while here you're consider only those that have both
addresses link-local.
Wouldn't it be better to plainly reject configuration that has one
address link-local but not the other one?
Jiri
On 03/14/2017 04:28 PM, Jiri Benc wrote:
> On Fri, 10 Mar 2017 23:39:44 +0100, Matthias Schiffer wrote:
>> @@ -233,17 +234,30 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, __be32 vni)
>> vni = 0;
>>
>> hlist_for_each_entry_rcu(vxlan, vni_head(vs, vni), hlist) {
>> - if (vxlan->default_dst.remote_vni == vni)
>> - return vxlan;
>> + if (vxlan->default_dst.remote_vni != vni)
>> + continue;
>> +
>> + if (IS_ENABLED(CONFIG_IPV6)) {
>> + const struct vxlan_config *cfg = &vxlan->cfg;
>> +
>> + if (cfg->remote_ifindex != 0 &&
>> + cfg->remote_ifindex != ifindex &&
>> + cfg->saddr.sa.sa_family == AF_INET6 &&
>> + (ipv6_addr_type(&cfg->saddr.sin6.sin6_addr) &
>> + IPV6_ADDR_LINKLOCAL))
>
> Calculating this (especially ipv6_addr_type) on every received packet
> looks unnecessarily expensive. Just store the fact the the local
> address is link-local in a flag during config. And compare the flag
> first before considering remote_ifindex.
>
> This is especially important for lwtunnels which can have anything in
> the saddr and remote_ifindex, yet those fields are ignored and the
> vni 0 entry has to be returned. It also means that the link-local flag
> must not be set for lwtunnels.
>
>> +#if IS_ENABLED(CONFIG_IPV6)
>> + if (conf->remote_ifindex != tmp->cfg.remote_ifindex &&
>> + conf->saddr.sa.sa_family == AF_INET6 &&
>> + tmp->cfg.saddr.sa.sa_family == AF_INET6 &&
>> + (ipv6_addr_type(&conf->saddr.sin6.sin6_addr) &
>> + IPV6_ADDR_LINKLOCAL) &&
>> + (ipv6_addr_type(&tmp->cfg.saddr.sin6.sin6_addr) &
>> + IPV6_ADDR_LINKLOCAL))
>> + continue;
>> +#endif
>
> In patch 1, you're checking for either source and destination address
> being link-local, while here you're consider only those that have both
> addresses link-local.
>
> Wouldn't it be better to plainly reject configuration that has one
> address link-local but not the other one?
While ensuring that the destination address is link-local iff the source
address is would also be an option, it didn't seem too useful as the
destination address will be a multicast address anyways in "normal" VXLAN
configurations. If we really want to check this, I guess the valid
combinations are:
source link-local - destination link-local UC
source link-local - destination link-local MC
source global/... - destination global/... UC
source global/... - destination any MC
Does this make sense?
>
> Jiri
Thanks for your comments, I'll send a v2 soon.
Matthias
On Wed, 15 Mar 2017 15:29:29 +0100, Matthias Schiffer wrote:
> While ensuring that the destination address is link-local iff the source
> address is would also be an option, it didn't seem too useful as the
> destination address will be a multicast address anyways in "normal" VXLAN
> configurations. If we really want to check this, I guess the valid
> combinations are:
>
> source link-local - destination link-local UC
> source link-local - destination link-local MC
> source global/... - destination global/... UC
> source global/... - destination any MC
>
> Does this make sense?
It does.
Thanks!
Jiri
On 03/15/2017 04:22 PM, Jiri Benc wrote:
> On Wed, 15 Mar 2017 15:29:29 +0100, Matthias Schiffer wrote:
>> While ensuring that the destination address is link-local iff the source
>> address is would also be an option, it didn't seem too useful as the
>> destination address will be a multicast address anyways in "normal" VXLAN
>> configurations. If we really want to check this, I guess the valid
>> combinations are:
>>
>> source link-local - destination link-local UC
>> source link-local - destination link-local MC
>> source global/... - destination global/... UC
>> source global/... - destination any MC
>>
>> Does this make sense?
>
> It does.
>
> Thanks!
>
> Jiri
>
While trying to integrate the additional checks, I noticed that the
vxlan_dev_configure() function has some serious issues in the changelink
case. An (probably incomplete) list of things that can go wrong:
- vxlan_dev_configure may return with an error as late the "if (conf->mtu)"
branch, after some settings have already been applied to the vxlan_dev or
net_device, leading to partial application in some error cases
- at the moment, changelink is allowed to change the address family of the
source/destionation addresses, but VXLAN_F_IPV6 is never removed, and
sockets are not recreated; I think changing the AF should just be disallowed
- conf->mtu will be re-applied in changelink even when the lowerdev has not
changed, possibly overwriting other MTU changes that have been made after
device creation (having conf->mtu in addition to dev->mtu might be a bad
idea in general?)
Each of the issues could be fixed separately, but at the moment, I'm rather
considering cleaning up the code by factoring out a vxlan_cfg_validate()
from vxlan_dev_configure(), to clearly separate validation and application
of the configuration. Is this the way to go, or do you have other suggestions?
-- Matthias