Re: [PATCH net-next] geneve: add rtnl changelink support

[Girish Moodalbail]

TL DR; There is indeed a race between geneve_changelink() and geneve transmit 
path w.r.t attributes being changed and the old value of those attributes being 
used in the transmit patch. I will resubmit V2 of the patch with those issues 
addressed. Thanks!


Please see in-line for my other comments..




Signed-off-by: Girish Moodalbail 
---
 drivers/net/geneve.c | 149 ---
 1 file changed, 117 insertions(+), 32 deletions(-)


...

@@ -1169,45 +1181,58 @@ static void init_tnl_info(struct ip_tunnel_info *info, 
__u16 dst_port)
info->key.tp_dst = htons(dst_port);
 }

-static int geneve_newlink(struct net *net, struct net_device *dev,
- struct nlattr *tb[], struct nlattr *data[])
+static int geneve_nl2info(struct net_device *dev, struct nlattr *tb[],
+ struct nlattr *data[], struct ip_tunnel_info *info,
+ bool *metadata, bool *use_udp6_rx_checksums,
+ bool changelink)
 {
-   bool use_udp6_rx_checksums = false;
-   struct ip_tunnel_info info;
-   bool metadata = false;
+   struct geneve_dev *geneve = netdev_priv(dev);

-   init_tnl_info(, GENEVE_UDP_PORT);
+   if (changelink) {
+   /* if changelink operation, start with old existing info */
+   memcpy(info, >info, sizeof(*info));
+   *metadata = geneve->collect_md;
+   *use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
+   } else {
+   init_tnl_info(info, GENEVE_UDP_PORT);
+   }

if (data[IFLA_GENEVE_REMOTE] && data[IFLA_GENEVE_REMOTE6])
return -EINVAL;

if (data[IFLA_GENEVE_REMOTE]) {
-   info.key.u.ipv4.dst =
+   info->key.u.ipv4.dst =
nla_get_in_addr(data[IFLA_GENEVE_REMOTE]);

-   if (IN_MULTICAST(ntohl(info.key.u.ipv4.dst))) {
+   if (IN_MULTICAST(ntohl(info->key.u.ipv4.dst))) {
netdev_dbg(dev, "multicast remote is unsupported\n");
return -EINVAL;
}
+   if (changelink &&
+   ip_tunnel_info_af(>info) == AF_INET6) {
+   info->mode &= ~IP_TUNNEL_INFO_IPV6;
+   info->key.tun_flags &= ~TUNNEL_CSUM;
+   *use_udp6_rx_checksums = false;
+   }

This allows changelink to change ipv4 address but there are no changes
made to the geneve tunnel port hash table after this update.


The following code in geneve_changelink() does what you are asking for

+if (!geneve_dst_addr_equal(>info, ))
+dst_cache_reset(_cache);

geneve_nl2info() accrues all the allowed changes to be made and captures it in 
ip_tunnel_info structure and then the above code in geneve_changelink() ensures 
that all the route cache associated with the old remote address are released 
when the next lookup occurs.



We also
need to check to see if there is any conflicts with existing ports.


This is not needed since we don't support changing the remote port.



What is the barrier between the rx/tx threads and changelink process?


There is an issue here like you pointed out (thanks!). Will fix that issue.




}

if (data[IFLA_GENEVE_REMOTE6]) {
  #if IS_ENABLED(CONFIG_IPV6)
-   info.mode = IP_TUNNEL_INFO_IPV6;
-   info.key.u.ipv6.dst =
+   info->mode = IP_TUNNEL_INFO_IPV6;
+   info->key.u.ipv6.dst =
nla_get_in6_addr(data[IFLA_GENEVE_REMOTE6]);

-   if (ipv6_addr_type() &
+   if (ipv6_addr_type(>key.u.ipv6.dst) &
IPV6_ADDR_LINKLOCAL) {
netdev_dbg(dev, "link-local remote is unsupported\n");
return -EINVAL;
}
-   if (ipv6_addr_is_multicast()) {
+   if (ipv6_addr_is_multicast(>key.u.ipv6.dst)) {
netdev_dbg(dev, "multicast remote is unsupported\n");
return -EINVAL;
}
-   info.key.tun_flags |= TUNNEL_CSUM;
-   use_udp6_rx_checksums = true;
+   info->key.tun_flags |= TUNNEL_CSUM;
+   *use_udp6_rx_checksums = true;

Same here. We need to check/fix the geneve tunnel hash table according
to new IP address.


This is taken care by the call to dst_cache_reset() whenever the remote address 
changes. This function already takes care of races and contentions


8<-8<--
/**
 *  dst_cache_reset - invalidate the cache contents
 *  @dst_cache: the cache
 *
 *  This do not free the cached dst to avoid races and contentions.
 *  the dst will be freed on later cache lookup.
 */
static inline void dst_cache_reset(struct dst_cache *dst_cache)
{
dst_cache->reset_ts = jiffies;
}

Re: [PATCH net-next] geneve: add rtnl changelink support

[Girish Moodalbail]

On 5/16/17 12:31 PM, David Miller wrote:

From: Girish Moodalbail 
Date: Mon, 15 May 2017 10:47:04 -0700


if (data[IFLA_GENEVE_REMOTE]) {
-   info.key.u.ipv4.dst =
+   info->key.u.ipv4.dst =
nla_get_in_addr(data[IFLA_GENEVE_REMOTE]);

-   if (IN_MULTICAST(ntohl(info.key.u.ipv4.dst))) {
+   if (IN_MULTICAST(ntohl(info->key.u.ipv4.dst))) {
netdev_dbg(dev, "multicast remote is unsupported\n");
return -EINVAL;
}
+   if (changelink &&
+   ip_tunnel_info_af(>info) == AF_INET6) {
+   info->mode &= ~IP_TUNNEL_INFO_IPV6;
+   info->key.tun_flags &= ~TUNNEL_CSUM;
+   *use_udp6_rx_checksums = false;
+   }
}


I don't understand this "changelink" guarded code, why do you need to
clear all of this state out if the existing tunnel type if AF_INET6
and only when doing a changelink?

In any event, I think you need to add a comment explaining it.



If geneve link was overlayed over IPv6 network and now the user modifies the 
link to be over IPv4 network by doing


# ip link set gen0 type geneve id 100 remote 192.168.13.2

Then we will need to

 - reset info->mode to be not IPv6 type
 - the default for UDP checksum over IPv4 is 'no', so reset that and
 - set use_udp6_rx_checksums to its default value which is false.

I will capture the above information concisely in a comment around that 
'changelink' guard.


thanks,
~Girish

Re: [PATCH net-next] geneve: add rtnl changelink support

[Pravin Shelar]

On Mon, May 15, 2017 at 10:47 AM, Girish Moodalbail
 wrote:
> This patch adds changelink rtnl operation support for geneve devices.
> Code changes involve:
>   - refactor geneve_newlink into geneve_nl2info to be used by both
> geneve_newlink and geneve_changelink
>   - geneve_nl2info takes a changelink boolean argument to isolate
> changelink checks and updates.
>   - Allow changing only a few attributes:
> - return -EOPNOTSUPP for attributes that cannot be changed for
>   now. Incremental patches can make the non-supported one
>   available in the future if needed.
>
Thanks for working on this.

> Signed-off-by: Girish Moodalbail 
> ---
>  drivers/net/geneve.c | 149 
> ---
>  1 file changed, 117 insertions(+), 32 deletions(-)
>
...
> @@ -1169,45 +1181,58 @@ static void init_tnl_info(struct ip_tunnel_info 
> *info, __u16 dst_port)
> info->key.tp_dst = htons(dst_port);
>  }
>
> -static int geneve_newlink(struct net *net, struct net_device *dev,
> - struct nlattr *tb[], struct nlattr *data[])
> +static int geneve_nl2info(struct net_device *dev, struct nlattr *tb[],
> + struct nlattr *data[], struct ip_tunnel_info *info,
> + bool *metadata, bool *use_udp6_rx_checksums,
> + bool changelink)
>  {
> -   bool use_udp6_rx_checksums = false;
> -   struct ip_tunnel_info info;
> -   bool metadata = false;
> +   struct geneve_dev *geneve = netdev_priv(dev);
>
> -   init_tnl_info(, GENEVE_UDP_PORT);
> +   if (changelink) {
> +   /* if changelink operation, start with old existing info */
> +   memcpy(info, >info, sizeof(*info));
> +   *metadata = geneve->collect_md;
> +   *use_udp6_rx_checksums = geneve->use_udp6_rx_checksums;
> +   } else {
> +   init_tnl_info(info, GENEVE_UDP_PORT);
> +   }
>
> if (data[IFLA_GENEVE_REMOTE] && data[IFLA_GENEVE_REMOTE6])
> return -EINVAL;
>
> if (data[IFLA_GENEVE_REMOTE]) {
> -   info.key.u.ipv4.dst =
> +   info->key.u.ipv4.dst =
> nla_get_in_addr(data[IFLA_GENEVE_REMOTE]);
>
> -   if (IN_MULTICAST(ntohl(info.key.u.ipv4.dst))) {
> +   if (IN_MULTICAST(ntohl(info->key.u.ipv4.dst))) {
> netdev_dbg(dev, "multicast remote is unsupported\n");
> return -EINVAL;
> }
> +   if (changelink &&
> +   ip_tunnel_info_af(>info) == AF_INET6) {
> +   info->mode &= ~IP_TUNNEL_INFO_IPV6;
> +   info->key.tun_flags &= ~TUNNEL_CSUM;
> +   *use_udp6_rx_checksums = false;
> +   }
This allows changelink to change ipv4 address but there are no changes
made to the geneve tunnel port hash table after this update. We also
need to check to see if there is any conflicts with existing ports.

What is the barrier between the rx/tx threads and changelink process?

> }
>
> if (data[IFLA_GENEVE_REMOTE6]) {
>   #if IS_ENABLED(CONFIG_IPV6)
> -   info.mode = IP_TUNNEL_INFO_IPV6;
> -   info.key.u.ipv6.dst =
> +   info->mode = IP_TUNNEL_INFO_IPV6;
> +   info->key.u.ipv6.dst =
> nla_get_in6_addr(data[IFLA_GENEVE_REMOTE6]);
>
> -   if (ipv6_addr_type() &
> +   if (ipv6_addr_type(>key.u.ipv6.dst) &
> IPV6_ADDR_LINKLOCAL) {
> netdev_dbg(dev, "link-local remote is unsupported\n");
> return -EINVAL;
> }
> -   if (ipv6_addr_is_multicast()) {
> +   if (ipv6_addr_is_multicast(>key.u.ipv6.dst)) {
> netdev_dbg(dev, "multicast remote is unsupported\n");
> return -EINVAL;
> }
> -   info.key.tun_flags |= TUNNEL_CSUM;
> -   use_udp6_rx_checksums = true;
> +   info->key.tun_flags |= TUNNEL_CSUM;
> +   *use_udp6_rx_checksums = true;
Same here. We need to check/fix the geneve tunnel hash table according
to new IP address.

>  #else
> return -EPFNOSUPPORT;
>  #endif
> @@ -1216,48 +1241,107 @@ static int geneve_newlink(struct net *net, struct 
> net_device *dev,
...
>
> -   if (data[IFLA_GENEVE_PORT])
> -   info.key.tp_dst = nla_get_be16(data[IFLA_GENEVE_PORT]);
> +   if (data[IFLA_GENEVE_PORT]) {
> +   if (changelink)
> +   return -EOPNOTSUPP;
> +   info->key.tp_dst = nla_get_be16(data[IFLA_GENEVE_PORT]);
> +   }
> +
> +   if (data[IFLA_GENEVE_COLLECT_METADATA]) {
> +   if (changelink)
> +   return -EOPNOTSUPP;
Rather 

Re: [PATCH net-next] geneve: add rtnl changelink support

[David Miller]

From: Girish Moodalbail 
Date: Mon, 15 May 2017 10:47:04 -0700

>   if (data[IFLA_GENEVE_REMOTE]) {
> - info.key.u.ipv4.dst =
> + info->key.u.ipv4.dst =
>   nla_get_in_addr(data[IFLA_GENEVE_REMOTE]);
>  
> - if (IN_MULTICAST(ntohl(info.key.u.ipv4.dst))) {
> + if (IN_MULTICAST(ntohl(info->key.u.ipv4.dst))) {
>   netdev_dbg(dev, "multicast remote is unsupported\n");
>   return -EINVAL;
>   }
> + if (changelink &&
> + ip_tunnel_info_af(>info) == AF_INET6) {
> + info->mode &= ~IP_TUNNEL_INFO_IPV6;
> + info->key.tun_flags &= ~TUNNEL_CSUM;
> + *use_udp6_rx_checksums = false;
> + }
>   }

I don't understand this "changelink" guarded code, why do you need to
clear all of this state out if the existing tunnel type if AF_INET6
and only when doing a changelink?

In any event, I think you need to add a comment explaining it.

Re: [PATCH net-next] geneve: add rtnl changelink support

[David Miller]

From: Girish Moodalbail 
Date: Mon,  8 May 2017 13:08:24 -0700

> This patch adds changelink rtnl operation support for geneve devices.
> Code changes involve:
>   - refactor geneve_newlink into geneve_nl2info to be used by both
> geneve_newlink and geneve_changelink
>   - geneve_nl2info takes a changelink boolean argument to isolate
> changelink checks and updates.
>   - Allow changing only a few attributes:
> - return -EOPNOTSUPP for attributes that cannot be changed for
>   now. Incremental patches can make the non-supported one
>   available in the future if needed.
> 
> Signed-off-by: Girish Moodalbail 

The net-next tree is closed, please resubmit this when the net-next
tree opens back up.

Thank you.