* [PATCH net-next] geneve: add rtnl changelink support
@ 2017-05-15 17:47 Girish Moodalbail
2017-05-16 19:31 ` David Miller
2017-05-16 20:00 ` Pravin Shelar
0 siblings, 2 replies; 7+ messages in thread
From: Girish Moodalbail @ 2017-05-15 17:47 UTC (permalink / raw)
To: netdev; +Cc: davem, pshelar, joe, jbenc
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 <girish.moodalbail@oracle.com>
---
drivers/net/geneve.c | 149 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 117 insertions(+), 32 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index dec5d56..6528910 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1112,6 +1112,18 @@ static bool is_tnl_info_zero(const struct ip_tunnel_info *info)
return true;
}
+static inline bool geneve_dst_addr_equal(struct ip_tunnel_info *a,
+ struct ip_tunnel_info *b)
+{
+ if (ip_tunnel_info_af(a) != ip_tunnel_info_af(b))
+ return false;
+
+ if (ip_tunnel_info_af(a) == AF_INET)
+ return a->key.u.ipv4.dst == b->key.u.ipv4.dst;
+ else
+ return ipv6_addr_equal(&a->key.u.ipv6.dst, &b->key.u.ipv6.dst);
+}
+
static int geneve_configure(struct net *net, struct net_device *dev,
const struct ip_tunnel_info *info,
bool metadata, bool ipv6_rx_csum)
@@ -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(&info, GENEVE_UDP_PORT);
+ if (changelink) {
+ /* if changelink operation, start with old existing info */
+ memcpy(info, &geneve->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(&geneve->info) == AF_INET6) {
+ info->mode &= ~IP_TUNNEL_INFO_IPV6;
+ info->key.tun_flags &= ~TUNNEL_CSUM;
+ *use_udp6_rx_checksums = false;
+ }
}
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(&info.key.u.ipv6.dst) &
+ if (ipv6_addr_type(&info->key.u.ipv6.dst) &
IPV6_ADDR_LINKLOCAL) {
netdev_dbg(dev, "link-local remote is unsupported\n");
return -EINVAL;
}
- if (ipv6_addr_is_multicast(&info.key.u.ipv6.dst)) {
+ if (ipv6_addr_is_multicast(&info->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;
#else
return -EPFNOSUPPORT;
#endif
@@ -1216,48 +1241,107 @@ static int geneve_newlink(struct net *net, struct net_device *dev,
if (data[IFLA_GENEVE_ID]) {
__u32 vni;
__u8 tvni[3];
+ __be64 tunid;
vni = nla_get_u32(data[IFLA_GENEVE_ID]);
tvni[0] = (vni & 0x00ff0000) >> 16;
tvni[1] = (vni & 0x0000ff00) >> 8;
tvni[2] = vni & 0x000000ff;
- info.key.tun_id = vni_to_tunnel_id(tvni);
+ tunid = vni_to_tunnel_id(tvni);
+ if (changelink && (tunid != info->key.tun_id))
+ return -EOPNOTSUPP;
+ info->key.tun_id = tunid;
}
+
if (data[IFLA_GENEVE_TTL])
- info.key.ttl = nla_get_u8(data[IFLA_GENEVE_TTL]);
+ info->key.ttl = nla_get_u8(data[IFLA_GENEVE_TTL]);
if (data[IFLA_GENEVE_TOS])
- info.key.tos = nla_get_u8(data[IFLA_GENEVE_TOS]);
+ info->key.tos = nla_get_u8(data[IFLA_GENEVE_TOS]);
if (data[IFLA_GENEVE_LABEL]) {
- info.key.label = nla_get_be32(data[IFLA_GENEVE_LABEL]) &
+ info->key.label = nla_get_be32(data[IFLA_GENEVE_LABEL]) &
IPV6_FLOWLABEL_MASK;
- if (info.key.label && (!(info.mode & IP_TUNNEL_INFO_IPV6)))
+ if (info->key.label && (!(info->mode & IP_TUNNEL_INFO_IPV6)))
return -EINVAL;
}
- 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;
+ *metadata = true;
+ }
+
+ if (data[IFLA_GENEVE_UDP_CSUM]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
+ info->key.tun_flags |= TUNNEL_CSUM;
+ }
+
+ if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
+ info->key.tun_flags &= ~TUNNEL_CSUM;
+ }
- if (data[IFLA_GENEVE_COLLECT_METADATA])
- metadata = true;
+ if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
+ *use_udp6_rx_checksums = false;
+ }
- if (data[IFLA_GENEVE_UDP_CSUM] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
- info.key.tun_flags |= TUNNEL_CSUM;
+ return 0;
+}
- if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
- info.key.tun_flags &= ~TUNNEL_CSUM;
+static int geneve_newlink(struct net *net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+ bool use_udp6_rx_checksums = false;
+ struct ip_tunnel_info info;
+ bool metadata = false;
+ int err;
- if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
- use_udp6_rx_checksums = false;
+ err = geneve_nl2info(dev, tb, data, &info, &metadata,
+ &use_udp6_rx_checksums, false);
+ if (err)
+ return err;
return geneve_configure(net, dev, &info, metadata, use_udp6_rx_checksums);
}
+static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
+ struct nlattr *data[])
+{
+ struct geneve_dev *geneve = netdev_priv(dev);
+ struct ip_tunnel_info info;
+ bool metadata = false;
+ bool use_udp6_rx_checksums = false;
+ int err;
+
+ err = geneve_nl2info(dev, tb, data, &info, &metadata,
+ &use_udp6_rx_checksums, true);
+ if (err)
+ return err;
+
+ if (!geneve_dst_addr_equal(&geneve->info, &info))
+ dst_cache_reset(&info.dst_cache);
+ geneve->info = info;
+ geneve->collect_md = metadata;
+ geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
+
+ return 0;
+}
+
static void geneve_dellink(struct net_device *dev, struct list_head *head)
{
struct geneve_dev *geneve = netdev_priv(dev);
@@ -1344,6 +1428,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
.setup = geneve_setup,
.validate = geneve_validate,
.newlink = geneve_newlink,
+ .changelink = geneve_changelink,
.dellink = geneve_dellink,
.get_size = geneve_get_size,
.fill_info = geneve_fill_info,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] geneve: add rtnl changelink support
2017-05-15 17:47 [PATCH net-next] geneve: add rtnl changelink support Girish Moodalbail
@ 2017-05-16 19:31 ` David Miller
2017-05-16 20:09 ` Girish Moodalbail
2017-05-16 20:00 ` Pravin Shelar
1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2017-05-16 19:31 UTC (permalink / raw)
To: girish.moodalbail; +Cc: netdev, pshelar, joe, jbenc
From: Girish Moodalbail <girish.moodalbail@oracle.com>
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(&geneve->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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] geneve: add rtnl changelink support
2017-05-15 17:47 [PATCH net-next] geneve: add rtnl changelink support Girish Moodalbail
2017-05-16 19:31 ` David Miller
@ 2017-05-16 20:00 ` Pravin Shelar
2017-05-18 15:15 ` Girish Moodalbail
1 sibling, 1 reply; 7+ messages in thread
From: Pravin Shelar @ 2017-05-16 20:00 UTC (permalink / raw)
To: Girish Moodalbail
Cc: Linux Kernel Network Developers, David S. Miller, Joe Stringer,
Jiri Benc
On Mon, May 15, 2017 at 10:47 AM, Girish Moodalbail
<girish.moodalbail@oracle.com> 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 <girish.moodalbail@oracle.com>
> ---
> 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(&info, GENEVE_UDP_PORT);
> + if (changelink) {
> + /* if changelink operation, start with old existing info */
> + memcpy(info, &geneve->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(&geneve->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(&info.key.u.ipv6.dst) &
> + if (ipv6_addr_type(&info->key.u.ipv6.dst) &
> IPV6_ADDR_LINKLOCAL) {
> netdev_dbg(dev, "link-local remote is unsupported\n");
> return -EINVAL;
> }
> - if (ipv6_addr_is_multicast(&info.key.u.ipv6.dst)) {
> + if (ipv6_addr_is_multicast(&info->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 than blindly returning error here it should check if the
changelink is changing existing configuration.
> + *metadata = true;
> + }
> +
> + if (data[IFLA_GENEVE_UDP_CSUM]) {
> + if (changelink)
> + return -EOPNOTSUPP;
> + if (nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
> + info->key.tun_flags |= TUNNEL_CSUM;
> + }
> +
same here.
> + if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]) {
> + if (changelink)
> + return -EOPNOTSUPP;
> + if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
> + info->key.tun_flags &= ~TUNNEL_CSUM;
same here.
> + }
>
> - if (data[IFLA_GENEVE_COLLECT_METADATA])
> - metadata = true;
> + if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]) {
> + if (changelink)
> + return -EOPNOTSUPP;
> + if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
> + *use_udp6_rx_checksums = false;
> + }
>
> - if (data[IFLA_GENEVE_UDP_CSUM] &&
> - nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
> - info.key.tun_flags |= TUNNEL_CSUM;
> + return 0;
> +}
>
> - if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] &&
> - nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
> - info.key.tun_flags &= ~TUNNEL_CSUM;
> +static int geneve_newlink(struct net *net, struct net_device *dev,
> + struct nlattr *tb[], struct nlattr *data[])
> +{
> + bool use_udp6_rx_checksums = false;
> + struct ip_tunnel_info info;
> + bool metadata = false;
> + int err;
>
> - if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] &&
> - nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
> - use_udp6_rx_checksums = false;
> + err = geneve_nl2info(dev, tb, data, &info, &metadata,
> + &use_udp6_rx_checksums, false);
> + if (err)
> + return err;
>
> return geneve_configure(net, dev, &info, metadata, use_udp6_rx_checksums);
> }
>
> +static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
> + struct nlattr *data[])
> +{
> + struct geneve_dev *geneve = netdev_priv(dev);
> + struct ip_tunnel_info info;
> + bool metadata = false;
> + bool use_udp6_rx_checksums = false;
> + int err;
> +
> + err = geneve_nl2info(dev, tb, data, &info, &metadata,
> + &use_udp6_rx_checksums, true);
> + if (err)
> + return err;
> +
> + if (!geneve_dst_addr_equal(&geneve->info, &info))
> + dst_cache_reset(&info.dst_cache);
> + geneve->info = info;
This would just overwrite dst-cache, which could leak the percpu
cached dst-entry objects.
> + geneve->collect_md = metadata;
> + geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
> +
> + return 0;
> +}
> +
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] geneve: add rtnl changelink support
2017-05-16 19:31 ` David Miller
@ 2017-05-16 20:09 ` Girish Moodalbail
0 siblings, 0 replies; 7+ messages in thread
From: Girish Moodalbail @ 2017-05-16 20:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, pshelar, joe, jbenc
On 5/16/17 12:31 PM, David Miller wrote:
> From: Girish Moodalbail <girish.moodalbail@oracle.com>
> 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(&geneve->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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] geneve: add rtnl changelink support
2017-05-16 20:00 ` Pravin Shelar
@ 2017-05-18 15:15 ` Girish Moodalbail
0 siblings, 0 replies; 7+ messages in thread
From: Girish Moodalbail @ 2017-05-18 15:15 UTC (permalink / raw)
To: Pravin Shelar
Cc: Linux Kernel Network Developers, David S. Miller, Joe Stringer,
Jiri Benc
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 <girish.moodalbail@oracle.com>
>> ---
>> 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(&info, GENEVE_UDP_PORT);
>> + if (changelink) {
>> + /* if changelink operation, start with old existing info */
>> + memcpy(info, &geneve->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(&geneve->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(&geneve->info, &info))
+ dst_cache_reset(&info.dst_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(&info.key.u.ipv6.dst) &
>> + if (ipv6_addr_type(&info->key.u.ipv6.dst) &
>> IPV6_ADDR_LINKLOCAL) {
>> netdev_dbg(dev, "link-local remote is unsupported\n");
>> return -EINVAL;
>> }
>> - if (ipv6_addr_is_multicast(&info.key.u.ipv6.dst)) {
>> + if (ipv6_addr_is_multicast(&info->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;
}
------------8<-----------------8<------
.... by setting reset_ts to current value of jiffies. After this, when we call
dst_cache_get_ip4() to get a route entry for geneve packet we ensure that cache
is old/obsolete/invalid/incorrect through the following check in that function
------------8<-----------------8<------
if (unlikely(!time_after(idst->refresh_ts, dst_cache->reset_ts) ||
(dst->obsolete && !dst->ops->check(dst, idst->cookie)))) {
dst_cache_per_cpu_dst_set(idst, NULL, 0);
dst_release(dst);
goto fail;
}
------------8<-----------------8<------
>
>> #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 than blindly returning error here it should check if the
> changelink is changing existing configuration.
I would like to start by saying that I have kept the same semantic as
vxlan_changelink() here to keep the operations consistent across geneve and
vxlan links.
Furthermore, will this not give a semantic that we do support changing metadata
to an user? For example: Assume that there already existed geneve datalink with
metadata enabled, now if I do
# ip link set gen0 type geneve id 100 metadata
it will return success giving an idea that '[no]metadata' attribute is
modifiable. So, when they try to do
# ip link set gen0 type geneve id 100 nometadata
it will fail.
>
>> + *metadata = true;
>> + }
>> +
>> + if (data[IFLA_GENEVE_UDP_CSUM]) {
>> + if (changelink)
>> + return -EOPNOTSUPP;
>> + if (nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
>> + info->key.tun_flags |= TUNNEL_CSUM;
>> + }
>> +
> same here.
please see above
>
>> + if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]) {
>> + if (changelink)
>> + return -EOPNOTSUPP;
>> + if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
>> + info->key.tun_flags &= ~TUNNEL_CSUM;
> same here.
please see above
>
>> + }
>>
>> - if (data[IFLA_GENEVE_COLLECT_METADATA])
>> - metadata = true;
>> + if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]) {
>> + if (changelink)
>> + return -EOPNOTSUPP;
>> + if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
>> + *use_udp6_rx_checksums = false;
>> + }
>>
>> - if (data[IFLA_GENEVE_UDP_CSUM] &&
>> - nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
>> - info.key.tun_flags |= TUNNEL_CSUM;
>> + return 0;
>> +}
>>
>> - if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] &&
>> - nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
>> - info.key.tun_flags &= ~TUNNEL_CSUM;
>> +static int geneve_newlink(struct net *net, struct net_device *dev,
>> + struct nlattr *tb[], struct nlattr *data[])
>> +{
>> + bool use_udp6_rx_checksums = false;
>> + struct ip_tunnel_info info;
>> + bool metadata = false;
>> + int err;
>>
>> - if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] &&
>> - nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
>> - use_udp6_rx_checksums = false;
>> + err = geneve_nl2info(dev, tb, data, &info, &metadata,
>> + &use_udp6_rx_checksums, false);
>> + if (err)
>> + return err;
>>
>> return geneve_configure(net, dev, &info, metadata, use_udp6_rx_checksums);
>> }
>>
>> +static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
>> + struct nlattr *data[])
>> +{
>> + struct geneve_dev *geneve = netdev_priv(dev);
>> + struct ip_tunnel_info info;
>> + bool metadata = false;
>> + bool use_udp6_rx_checksums = false;
>> + int err;
>> +
>> + err = geneve_nl2info(dev, tb, data, &info, &metadata,
>> + &use_udp6_rx_checksums, true);
>> + if (err)
>> + return err;
>> +
>> + if (!geneve_dst_addr_equal(&geneve->info, &info))
>> + dst_cache_reset(&info.dst_cache);
>> + geneve->info = info;
> This would just overwrite dst-cache, which could leak the percpu
> cached dst-entry objects.
It will not. The ip_tunnel_info`dst_cache is a struct with a pointer to per_cpu
struct and reset_ts. We copy this structure at the beginning of
geneve_changelink() and this is done under rtnl_lock(). That
ip_tunnel_info`dst_cache`per_cpu struct doesn't change throughout the operation
but only reset_ts is set to 'jiffies' if the remote address changes.
Thanks,
~Girish
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] geneve: add rtnl changelink support
2017-05-08 20:08 Girish Moodalbail
@ 2017-05-08 20:18 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2017-05-08 20:18 UTC (permalink / raw)
To: girish.moodalbail; +Cc: netdev, pshelar, joe, jbenc
From: Girish Moodalbail <girish.moodalbail@oracle.com>
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 <girish.moodalbail@oracle.com>
The net-next tree is closed, please resubmit this when the net-next
tree opens back up.
Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next] geneve: add rtnl changelink support
@ 2017-05-08 20:08 Girish Moodalbail
2017-05-08 20:18 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Girish Moodalbail @ 2017-05-08 20:08 UTC (permalink / raw)
To: netdev; +Cc: davem, pshelar, joe, jbenc
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 <girish.moodalbail@oracle.com>
---
drivers/net/geneve.c | 149 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 117 insertions(+), 32 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index dec5d56..6528910 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1112,6 +1112,18 @@ static bool is_tnl_info_zero(const struct ip_tunnel_info *info)
return true;
}
+static inline bool geneve_dst_addr_equal(struct ip_tunnel_info *a,
+ struct ip_tunnel_info *b)
+{
+ if (ip_tunnel_info_af(a) != ip_tunnel_info_af(b))
+ return false;
+
+ if (ip_tunnel_info_af(a) == AF_INET)
+ return a->key.u.ipv4.dst == b->key.u.ipv4.dst;
+ else
+ return ipv6_addr_equal(&a->key.u.ipv6.dst, &b->key.u.ipv6.dst);
+}
+
static int geneve_configure(struct net *net, struct net_device *dev,
const struct ip_tunnel_info *info,
bool metadata, bool ipv6_rx_csum)
@@ -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(&info, GENEVE_UDP_PORT);
+ if (changelink) {
+ /* if changelink operation, start with old existing info */
+ memcpy(info, &geneve->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(&geneve->info) == AF_INET6) {
+ info->mode &= ~IP_TUNNEL_INFO_IPV6;
+ info->key.tun_flags &= ~TUNNEL_CSUM;
+ *use_udp6_rx_checksums = false;
+ }
}
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(&info.key.u.ipv6.dst) &
+ if (ipv6_addr_type(&info->key.u.ipv6.dst) &
IPV6_ADDR_LINKLOCAL) {
netdev_dbg(dev, "link-local remote is unsupported\n");
return -EINVAL;
}
- if (ipv6_addr_is_multicast(&info.key.u.ipv6.dst)) {
+ if (ipv6_addr_is_multicast(&info->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;
#else
return -EPFNOSUPPORT;
#endif
@@ -1216,48 +1241,107 @@ static int geneve_newlink(struct net *net, struct net_device *dev,
if (data[IFLA_GENEVE_ID]) {
__u32 vni;
__u8 tvni[3];
+ __be64 tunid;
vni = nla_get_u32(data[IFLA_GENEVE_ID]);
tvni[0] = (vni & 0x00ff0000) >> 16;
tvni[1] = (vni & 0x0000ff00) >> 8;
tvni[2] = vni & 0x000000ff;
- info.key.tun_id = vni_to_tunnel_id(tvni);
+ tunid = vni_to_tunnel_id(tvni);
+ if (changelink && (tunid != info->key.tun_id))
+ return -EOPNOTSUPP;
+ info->key.tun_id = tunid;
}
+
if (data[IFLA_GENEVE_TTL])
- info.key.ttl = nla_get_u8(data[IFLA_GENEVE_TTL]);
+ info->key.ttl = nla_get_u8(data[IFLA_GENEVE_TTL]);
if (data[IFLA_GENEVE_TOS])
- info.key.tos = nla_get_u8(data[IFLA_GENEVE_TOS]);
+ info->key.tos = nla_get_u8(data[IFLA_GENEVE_TOS]);
if (data[IFLA_GENEVE_LABEL]) {
- info.key.label = nla_get_be32(data[IFLA_GENEVE_LABEL]) &
+ info->key.label = nla_get_be32(data[IFLA_GENEVE_LABEL]) &
IPV6_FLOWLABEL_MASK;
- if (info.key.label && (!(info.mode & IP_TUNNEL_INFO_IPV6)))
+ if (info->key.label && (!(info->mode & IP_TUNNEL_INFO_IPV6)))
return -EINVAL;
}
- 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;
+ *metadata = true;
+ }
+
+ if (data[IFLA_GENEVE_UDP_CSUM]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
+ info->key.tun_flags |= TUNNEL_CSUM;
+ }
+
+ if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
+ info->key.tun_flags &= ~TUNNEL_CSUM;
+ }
- if (data[IFLA_GENEVE_COLLECT_METADATA])
- metadata = true;
+ if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]) {
+ if (changelink)
+ return -EOPNOTSUPP;
+ if (nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
+ *use_udp6_rx_checksums = false;
+ }
- if (data[IFLA_GENEVE_UDP_CSUM] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_CSUM]))
- info.key.tun_flags |= TUNNEL_CSUM;
+ return 0;
+}
- if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]))
- info.key.tun_flags &= ~TUNNEL_CSUM;
+static int geneve_newlink(struct net *net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+ bool use_udp6_rx_checksums = false;
+ struct ip_tunnel_info info;
+ bool metadata = false;
+ int err;
- if (data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] &&
- nla_get_u8(data[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]))
- use_udp6_rx_checksums = false;
+ err = geneve_nl2info(dev, tb, data, &info, &metadata,
+ &use_udp6_rx_checksums, false);
+ if (err)
+ return err;
return geneve_configure(net, dev, &info, metadata, use_udp6_rx_checksums);
}
+static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
+ struct nlattr *data[])
+{
+ struct geneve_dev *geneve = netdev_priv(dev);
+ struct ip_tunnel_info info;
+ bool metadata = false;
+ bool use_udp6_rx_checksums = false;
+ int err;
+
+ err = geneve_nl2info(dev, tb, data, &info, &metadata,
+ &use_udp6_rx_checksums, true);
+ if (err)
+ return err;
+
+ if (!geneve_dst_addr_equal(&geneve->info, &info))
+ dst_cache_reset(&info.dst_cache);
+ geneve->info = info;
+ geneve->collect_md = metadata;
+ geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
+
+ return 0;
+}
+
static void geneve_dellink(struct net_device *dev, struct list_head *head)
{
struct geneve_dev *geneve = netdev_priv(dev);
@@ -1344,6 +1428,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
.setup = geneve_setup,
.validate = geneve_validate,
.newlink = geneve_newlink,
+ .changelink = geneve_changelink,
.dellink = geneve_dellink,
.get_size = geneve_get_size,
.fill_info = geneve_fill_info,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-05-18 15:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-15 17:47 [PATCH net-next] geneve: add rtnl changelink support Girish Moodalbail
2017-05-16 19:31 ` David Miller
2017-05-16 20:09 ` Girish Moodalbail
2017-05-16 20:00 ` Pravin Shelar
2017-05-18 15:15 ` Girish Moodalbail
-- strict thread matches above, loose matches on Subject: below --
2017-05-08 20:08 Girish Moodalbail
2017-05-08 20:18 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).