| Message ID | 49cd788f13c2cd3f6a42f34c219c9511cc1f9cec.1492187126.git.mschiffer@universe-factory.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 04/14/2017 07:44 PM, Matthias Schiffer wrote: > * Multicast addresses are never valid as local address > * Link-local IPv6 unicast addresses may only be used as remote when the > local address is link-local as well > * Don't allow link-local IPv6 local/remote addresses without interface > > We also store in the flags field if link-local addresses are used for the > follow-up patches that actually make VXLAN over link-local IPv6 work. > > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> > --- > > v2: was "vxlan: don't allow link-local IPv6 local/remote addresses without > interface" before. v2 does a lot more checks and adds the > VXLAN_F_IPV6_LINKLOCAL flag. > > drivers/net/vxlan.c | 35 +++++++++++++++++++++++++++++++++++ > include/net/vxlan.h | 2 ++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index 07f89b037681..95a71546e8f2 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, > if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) > return -EINVAL; > > + if (vxlan_addr_multicast(&conf->saddr)) > + return -EINVAL; > + > if (conf->saddr.sa.sa_family == AF_INET6) { > if (!IS_ENABLED(CONFIG_IPV6)) > return -EPFNOSUPPORT; > use_ipv6 = true; > conf->flags |= VXLAN_F_IPV6; > + > + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { > + int local_type = > + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); > + int remote_type = > + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); > + > + if (local_type & IPV6_ADDR_LINKLOCAL) { > + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && > + (remote_type != IPV6_ADDR_ANY)) { > + pr_info("invalid combination of address scopes\n"); Maybe pr_err()? > + return -EINVAL; > + } > + > + conf->flags |= VXLAN_F_IPV6_LINKLOCAL; > + } else { > + if (remote_type == > + (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) { > + pr_info("invalid combination of address scopes\n"); Here as well... > + return -EINVAL; > + } > + > + conf->flags &= ~VXLAN_F_IPV6_LINKLOCAL; > + } > + } > } > > if (conf->label && !use_ipv6) { [...] MBR, Sergei
On Fri, 14 Apr 2017 18:44:44 +0200 Matthias Schiffer <mschiffer@universe-factory.net> wrote: > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index 07f89b037681..95a71546e8f2 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, > if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) > return -EINVAL; > > + if (vxlan_addr_multicast(&conf->saddr)) > + return -EINVAL; > + > if (conf->saddr.sa.sa_family == AF_INET6) { > if (!IS_ENABLED(CONFIG_IPV6)) > return -EPFNOSUPPORT; > use_ipv6 = true; > conf->flags |= VXLAN_F_IPV6; > + > + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { > + int local_type = > + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); > + int remote_type = > + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); > + > + if (local_type & IPV6_ADDR_LINKLOCAL) { > + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && > + (remote_type != IPV6_ADDR_ANY)) { > + pr_info("invalid combination of address scopes\n"); It is always helpful to include device if possible in error message. netdev_notice(old->dev, " invalid combination of address scopes\n"); Also vxlan is good candidate for extended netlink error reporting.
On 04/14/2017 07:27 PM, Sergei Shtylyov wrote: > On 04/14/2017 07:44 PM, Matthias Schiffer wrote: > >> * Multicast addresses are never valid as local address >> * Link-local IPv6 unicast addresses may only be used as remote when the >> local address is link-local as well >> * Don't allow link-local IPv6 local/remote addresses without interface >> >> We also store in the flags field if link-local addresses are used for the >> follow-up patches that actually make VXLAN over link-local IPv6 work. >> >> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> >> --- >> >> v2: was "vxlan: don't allow link-local IPv6 local/remote addresses without >> interface" before. v2 does a lot more checks and adds the >> VXLAN_F_IPV6_LINKLOCAL flag. >> >> drivers/net/vxlan.c | 35 +++++++++++++++++++++++++++++++++++ >> include/net/vxlan.h | 2 ++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c >> index 07f89b037681..95a71546e8f2 100644 >> --- a/drivers/net/vxlan.c >> +++ b/drivers/net/vxlan.c >> @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net >> *src_net, struct vxlan_config *conf, >> if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) >> return -EINVAL; >> >> + if (vxlan_addr_multicast(&conf->saddr)) >> + return -EINVAL; >> + >> if (conf->saddr.sa.sa_family == AF_INET6) { >> if (!IS_ENABLED(CONFIG_IPV6)) >> return -EPFNOSUPPORT; >> use_ipv6 = true; >> conf->flags |= VXLAN_F_IPV6; >> + >> + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { >> + int local_type = >> + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); >> + int remote_type = >> + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); >> + >> + if (local_type & IPV6_ADDR_LINKLOCAL) { >> + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && >> + (remote_type != IPV6_ADDR_ANY)) { >> + pr_info("invalid combination of address scopes\n"); > > Maybe pr_err()? Hmm, I mostly followed the style of the existing code, which uses pr_info for such messages. Also, these messages can be triggered by userspace, as they're diagnostics for the newlink/changelink operations; I'm not convinced that their importance justifies pr_err(). Generally, it seems unusual to me to use the kernel log for configuration diagnostics at all; just removing the messages would be another option. Stephen also mentioned "extended netlink error reporting", but I guess that can be done in another patchset. Matthias
On 04/14/2017 07:36 PM, Stephen Hemminger wrote: > On Fri, 14 Apr 2017 18:44:44 +0200 > Matthias Schiffer <mschiffer@universe-factory.net> wrote: > >> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c >> index 07f89b037681..95a71546e8f2 100644 >> --- a/drivers/net/vxlan.c >> +++ b/drivers/net/vxlan.c >> @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, >> if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) >> return -EINVAL; >> >> + if (vxlan_addr_multicast(&conf->saddr)) >> + return -EINVAL; >> + >> if (conf->saddr.sa.sa_family == AF_INET6) { >> if (!IS_ENABLED(CONFIG_IPV6)) >> return -EPFNOSUPPORT; >> use_ipv6 = true; >> conf->flags |= VXLAN_F_IPV6; >> + >> + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { >> + int local_type = >> + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); >> + int remote_type = >> + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); >> + >> + if (local_type & IPV6_ADDR_LINKLOCAL) { >> + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && >> + (remote_type != IPV6_ADDR_ANY)) { >> + pr_info("invalid combination of address scopes\n"); > > It is always helpful to include device if possible in error message. > netdev_notice(old->dev, " invalid combination of address scopes\n"); That makes sense, I'll change it in v3. > Also vxlan is good candidate for extended netlink error reporting. Can you point me to a piece of code that does this? Unless you insist, I wouldn't do it in this patchset, but I might implement the extended error reporting later. Matthias
On 4/16/17, 8:03 AM, Matthias Schiffer wrote: > On 04/14/2017 07:36 PM, Stephen Hemminger wrote: >> On Fri, 14 Apr 2017 18:44:44 +0200 >> Matthias Schiffer <mschiffer@universe-factory.net> wrote: >> >>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c >>> index 07f89b037681..95a71546e8f2 100644 >>> --- a/drivers/net/vxlan.c >>> +++ b/drivers/net/vxlan.c >>> @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, >>> if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) >>> return -EINVAL; >>> >>> + if (vxlan_addr_multicast(&conf->saddr)) >>> + return -EINVAL; >>> + >>> if (conf->saddr.sa.sa_family == AF_INET6) { >>> if (!IS_ENABLED(CONFIG_IPV6)) >>> return -EPFNOSUPPORT; >>> use_ipv6 = true; >>> conf->flags |= VXLAN_F_IPV6; >>> + >>> + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { >>> + int local_type = >>> + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); >>> + int remote_type = >>> + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); >>> + >>> + if (local_type & IPV6_ADDR_LINKLOCAL) { >>> + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && >>> + (remote_type != IPV6_ADDR_ANY)) { >>> + pr_info("invalid combination of address scopes\n"); >> It is always helpful to include device if possible in error message. >> netdev_notice(old->dev, " invalid combination of address scopes\n"); > That makes sense, I'll change it in v3. I think it should just return -EINVAL here since this is in response to a netlink call from user-space. I dont think we should print anything but like stephen says use the extended ack mechanism to propagate more information about the error. > >> Also vxlan is good candidate for extended netlink error reporting. > Can you point me to a piece of code that does this? Unless you insist, I > wouldn't do it in this patchset, but I might implement the extended error > reporting later. > For rtnetlink users (vxlan is one of them) this is still in the works... see patch "net: rtnetlink: plumb extended ack to doit function"
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 07f89b037681..95a71546e8f2 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2881,11 +2881,39 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, if (conf->saddr.sa.sa_family != conf->remote_ip.sa.sa_family) return -EINVAL; + if (vxlan_addr_multicast(&conf->saddr)) + return -EINVAL; + if (conf->saddr.sa.sa_family == AF_INET6) { if (!IS_ENABLED(CONFIG_IPV6)) return -EPFNOSUPPORT; use_ipv6 = true; conf->flags |= VXLAN_F_IPV6; + + if (!(conf->flags & VXLAN_F_COLLECT_METADATA)) { + int local_type = + ipv6_addr_type(&conf->saddr.sin6.sin6_addr); + int remote_type = + ipv6_addr_type(&conf->remote_ip.sin6.sin6_addr); + + if (local_type & IPV6_ADDR_LINKLOCAL) { + if (!(remote_type & IPV6_ADDR_LINKLOCAL) && + (remote_type != IPV6_ADDR_ANY)) { + pr_info("invalid combination of address scopes\n"); + return -EINVAL; + } + + conf->flags |= VXLAN_F_IPV6_LINKLOCAL; + } else { + if (remote_type == + (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL)) { + pr_info("invalid combination of address scopes\n"); + return -EINVAL; + } + + conf->flags &= ~VXLAN_F_IPV6_LINKLOCAL; + } + } } if (conf->label && !use_ipv6) { @@ -2920,6 +2948,13 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf, return -EINVAL; } +#if IS_ENABLED(CONFIG_IPV6) + if (conf->flags & VXLAN_F_IPV6_LINKLOCAL) { + pr_info("link-local local/remote addresses require interface to be specified\n"); + return -EINVAL; + } +#endif + *lower = NULL; } diff --git a/include/net/vxlan.h b/include/net/vxlan.h index 479bb75789ea..b816a0a6686e 100644 --- a/include/net/vxlan.h +++ b/include/net/vxlan.h @@ -258,6 +258,7 @@ struct vxlan_dev { #define VXLAN_F_REMCSUM_NOPARTIAL 0x1000 #define VXLAN_F_COLLECT_METADATA 0x2000 #define VXLAN_F_GPE 0x4000 +#define VXLAN_F_IPV6_LINKLOCAL 0x8000 /* Flags that are used in the receive path. These flags must match in * order for a socket to be shareable @@ -272,6 +273,7 @@ struct vxlan_dev { /* Flags that can be set together with VXLAN_F_GPE. */ #define VXLAN_F_ALLOWED_GPE (VXLAN_F_GPE | \ VXLAN_F_IPV6 | \ + VXLAN_F_IPV6_LINKLOCAL | \ VXLAN_F_UDP_ZERO_CSUM_TX | \ VXLAN_F_UDP_ZERO_CSUM6_TX | \ VXLAN_F_UDP_ZERO_CSUM6_RX | \
* Multicast addresses are never valid as local address * Link-local IPv6 unicast addresses may only be used as remote when the local address is link-local as well * Don't allow link-local IPv6 local/remote addresses without interface We also store in the flags field if link-local addresses are used for the follow-up patches that actually make VXLAN over link-local IPv6 work. Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- v2: was "vxlan: don't allow link-local IPv6 local/remote addresses without interface" before. v2 does a lot more checks and adds the VXLAN_F_IPV6_LINKLOCAL flag. drivers/net/vxlan.c | 35 +++++++++++++++++++++++++++++++++++ include/net/vxlan.h | 2 ++ 2 files changed, 37 insertions(+)