[PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[Petr Machata]

In commit c706863bc890 ("net: ip6_gre: always reports o_key to
userspace"), ip6gre and ip6gretap tunnels started reporting a TUNNEL_KEY
output flag even if one was not configured at the device.

When an okey-less ip6gre or ip6gretap netdevice is created, it initially
encapsulates the packets without okey. But any configuration change
(even a non-change such as setting TOS to an already-configured value)
then causes the okey flag from the reported configuration to be
circulated back to actual configuration. From that point on, the device
encapsulates packets with output key of 0.

The intention was to implement this behavior for ERSPAN devices, not for
all ip6gre devices. The ERSPAN netdevice should really have its own
fill_info callback. Add one.

Fixes: c706863bc890 ("net: ip6_gre: always reports o_key to userspace")
CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 65a4f96dc462..0a6087cffe54 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -2094,15 +2094,13 @@ static size_t ip6gre_get_size(const struct net_device *dev)
 		0;
 }
 
-static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
+static int __ip6gre_fill_info(struct sk_buff *skb,
+			      const struct net_device *dev,
+			      __be16 base_o_flags)
 {
 	struct ip6_tnl *t = netdev_priv(dev);
 	struct __ip6_tnl_parm *p = &t->parms;
-	__be16 o_flags = p->o_flags;
-
-	if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
-	    !p->collect_md)
-		o_flags |= TUNNEL_KEY;
+	__be16 o_flags = p->o_flags | base_o_flags;
 
 	if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
 	    nla_put_be16(skb, IFLA_GRE_IFLAGS,
@@ -2155,6 +2153,11 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	return -EMSGSIZE;
 }
 
+static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+	return __ip6gre_fill_info(skb, dev, 0);
+}
+
 static const struct nla_policy ip6gre_policy[IFLA_GRE_MAX + 1] = {
 	[IFLA_GRE_LINK]        = { .type = NLA_U32 },
 	[IFLA_GRE_IFLAGS]      = { .type = NLA_U16 },
@@ -2256,6 +2259,20 @@ static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
 	return 0;
 }
 
+static int ip6erspan_fill_info(struct sk_buff *skb,
+			       const struct net_device *dev)
+{
+	struct ip6_tnl *t = netdev_priv(dev);
+	struct __ip6_tnl_parm *p = &t->parms;
+	__be16 base_o_flags = 0;
+
+	if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
+	    !p->collect_md)
+		base_o_flags |= TUNNEL_KEY;
+
+	return __ip6gre_fill_info(skb, dev, base_o_flags);
+}
+
 static struct rtnl_link_ops ip6gre_link_ops __read_mostly = {
 	.kind		= "ip6gre",
 	.maxtype	= IFLA_GRE_MAX,
@@ -2295,7 +2312,7 @@ static struct rtnl_link_ops ip6erspan_tap_ops __read_mostly = {
 	.newlink	= ip6erspan_newlink,
 	.changelink	= ip6erspan_changelink,
 	.get_size	= ip6gre_get_size,
-	.fill_info	= ip6gre_fill_info,
+	.fill_info	= ip6erspan_fill_info,
 	.get_link_net	= ip6_tnl_get_link_net,
 };
 
-- 
2.4.11

Re: [PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[Lorenzo Bianconi]

> In commit c706863bc890 ("net: ip6_gre: always reports o_key to
> userspace"), ip6gre and ip6gretap tunnels started reporting a TUNNEL_KEY
> output flag even if one was not configured at the device.
> 
> When an okey-less ip6gre or ip6gretap netdevice is created, it initially
> encapsulates the packets without okey. But any configuration change
> (even a non-change such as setting TOS to an already-configured value)
> then causes the okey flag from the reported configuration to be
> circulated back to actual configuration. From that point on, the device
> encapsulates packets with output key of 0.
> 
> The intention was to implement this behavior for ERSPAN devices, not for
> all ip6gre devices. The ERSPAN netdevice should really have its own
> fill_info callback. Add one.

Hi Petr,

I was assuming erspan_ver is set just for erspan tunnels. In particular I guess
the issue is due to the default erspan_ver configuration done in
ip6gre_netlink_parms (commit 84581bdae9587).
What about adding a routine to set erspan_ver and moving it in
ip6erspan_newlink/ip6erspan_changelink? In this way erspan_ver will be
defined just for erspan tunnels.
Moreover do we have a similar issue for IFLA_GRE_ERSPAN_INDEX in
ip6gre_fill_info?

Something like:

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 65a4f96dc462..bb525abd860e 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1719,6 +1719,24 @@ static int ip6erspan_tap_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static void ip6erspan_set_version(struct nlattr *data[],
+				  struct __ip6_tnl_parm *parms)
+{
+	parms->erspan_ver = 1;
+	if (data[IFLA_GRE_ERSPAN_VER])
+		parms->erspan_ver = nla_get_u8(data[IFLA_GRE_ERSPAN_VER]);
+
+	if (parms->erspan_ver == 1) {
+		if (data[IFLA_GRE_ERSPAN_INDEX])
+			parms->index = nla_get_u32(data[IFLA_GRE_ERSPAN_INDEX]);
+	} else if (parms->erspan_ver == 2) {
+		if (data[IFLA_GRE_ERSPAN_DIR])
+			parms->dir = nla_get_u8(data[IFLA_GRE_ERSPAN_DIR]);
+		if (data[IFLA_GRE_ERSPAN_HWID])
+			parms->hwid = nla_get_u16(data[IFLA_GRE_ERSPAN_HWID]);
+	}
+}
+
 static void ip6gre_netlink_parms(struct nlattr *data[],
 				struct __ip6_tnl_parm *parms)
 {
@@ -1767,20 +1785,6 @@ static void ip6gre_netlink_parms(struct nlattr *data[],
 
 	if (data[IFLA_GRE_COLLECT_METADATA])
 		parms->collect_md = true;
-
-	parms->erspan_ver = 1;
-	if (data[IFLA_GRE_ERSPAN_VER])
-		parms->erspan_ver = nla_get_u8(data[IFLA_GRE_ERSPAN_VER]);
-
-	if (parms->erspan_ver == 1) {
-		if (data[IFLA_GRE_ERSPAN_INDEX])
-			parms->index = nla_get_u32(data[IFLA_GRE_ERSPAN_INDEX]);
-	} else if (parms->erspan_ver == 2) {
-		if (data[IFLA_GRE_ERSPAN_DIR])
-			parms->dir = nla_get_u8(data[IFLA_GRE_ERSPAN_DIR]);
-		if (data[IFLA_GRE_ERSPAN_HWID])
-			parms->hwid = nla_get_u16(data[IFLA_GRE_ERSPAN_HWID]);
-	}
 }
 
 static int ip6gre_tap_init(struct net_device *dev)
@@ -2203,6 +2207,7 @@ static int ip6erspan_newlink(struct net *src_net, struct net_device *dev,
 	int err;
 
 	ip6gre_netlink_parms(data, &nt->parms);
+	ip6erspan_set_version(data, &nt->parms);
 	ign = net_generic(net, ip6gre_net_id);
 
 	if (nt->parms.collect_md) {
@@ -2248,6 +2253,7 @@ static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
 	if (IS_ERR(t))
 		return PTR_ERR(t);
 
+	ip6erspan_set_version(data, &p);
 	ip6gre_tunnel_unlink_md(ign, t);
 	ip6gre_tunnel_unlink(ign, t);
 	ip6erspan_tnl_change(t, &p, !tb[IFLA_MTU]);

Does it fix reported issue?

Regards,
Lorenzo

> 
> Fixes: c706863bc890 ("net: ip6_gre: always reports o_key to userspace")
> CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  net/ipv6/ip6_gre.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 65a4f96dc462..0a6087cffe54 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -2094,15 +2094,13 @@ static size_t ip6gre_get_size(const struct net_device *dev)
>  		0;
>  }
>  
> -static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
> +static int __ip6gre_fill_info(struct sk_buff *skb,
> +			      const struct net_device *dev,
> +			      __be16 base_o_flags)
>  {
>  	struct ip6_tnl *t = netdev_priv(dev);
>  	struct __ip6_tnl_parm *p = &t->parms;
> -	__be16 o_flags = p->o_flags;
> -
> -	if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
> -	    !p->collect_md)
> -		o_flags |= TUNNEL_KEY;
> +	__be16 o_flags = p->o_flags | base_o_flags;
>  
>  	if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
>  	    nla_put_be16(skb, IFLA_GRE_IFLAGS,
> @@ -2155,6 +2153,11 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  	return -EMSGSIZE;
>  }
>  
> +static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
> +{
> +	return __ip6gre_fill_info(skb, dev, 0);
> +}
> +
>  static const struct nla_policy ip6gre_policy[IFLA_GRE_MAX + 1] = {
>  	[IFLA_GRE_LINK]        = { .type = NLA_U32 },
>  	[IFLA_GRE_IFLAGS]      = { .type = NLA_U16 },
> @@ -2256,6 +2259,20 @@ static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
>  	return 0;
>  }
>  
> +static int ip6erspan_fill_info(struct sk_buff *skb,
> +			       const struct net_device *dev)
> +{
> +	struct ip6_tnl *t = netdev_priv(dev);
> +	struct __ip6_tnl_parm *p = &t->parms;
> +	__be16 base_o_flags = 0;
> +
> +	if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
> +	    !p->collect_md)
> +		base_o_flags |= TUNNEL_KEY;
> +
> +	return __ip6gre_fill_info(skb, dev, base_o_flags);
> +}
> +
>  static struct rtnl_link_ops ip6gre_link_ops __read_mostly = {
>  	.kind		= "ip6gre",
>  	.maxtype	= IFLA_GRE_MAX,
> @@ -2295,7 +2312,7 @@ static struct rtnl_link_ops ip6erspan_tap_ops __read_mostly = {
>  	.newlink	= ip6erspan_newlink,
>  	.changelink	= ip6erspan_changelink,
>  	.get_size	= ip6gre_get_size,
> -	.fill_info	= ip6gre_fill_info,
> +	.fill_info	= ip6erspan_fill_info,
>  	.get_link_net	= ip6_tnl_get_link_net,
>  };
>  
> -- 
> 2.4.11
> 

Re: [PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Wed, 13 Feb 2019 19:31:32 +0000

> In commit c706863bc890 ("net: ip6_gre: always reports o_key to
> userspace"), ip6gre and ip6gretap tunnels started reporting a TUNNEL_KEY
> output flag even if one was not configured at the device.
> 
> When an okey-less ip6gre or ip6gretap netdevice is created, it initially
> encapsulates the packets without okey. But any configuration change
> (even a non-change such as setting TOS to an already-configured value)
> then causes the okey flag from the reported configuration to be
> circulated back to actual configuration. From that point on, the device
> encapsulates packets with output key of 0.
> 
> The intention was to implement this behavior for ERSPAN devices, not for
> all ip6gre devices. The ERSPAN netdevice should really have its own
> fill_info callback. Add one.
> 
> Fixes: c706863bc890 ("net: ip6_gre: always reports o_key to userspace")
> CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

This commit you are fixing exists in the 'net' tree, therefore this is
a bug fix and should be targetted at 'net'.

Re: [PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[Petr Machata]

Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:

> Does it fix reported issue?

It does. Can you please formally send this? I'll retest and add my
Tested-by.

Thanks,
Petr

Re: [PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[Petr Machata]

David Miller <davem@davemloft.net> writes:

> From: Petr Machata <petrm@mellanox.com>
>
>> Fixes: c706863bc890 ("net: ip6_gre: always reports o_key to userspace")
>> CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>
> This commit you are fixing exists in the 'net' tree, therefore this is
> a bug fix and should be targetted at 'net'.

My mistake, I misread the follows-precedes tags at the commit.

Lorenzo, when you send your version, please send it to net.

Thanks,
Petr

Re: [PATCH net-next] net: ip6_gre: Give ERSPAN a fill_info link op of its own

[lorenzo.bianconi]

> 
> David Miller <davem@davemloft.net> writes:
> 
> > From: Petr Machata <petrm@mellanox.com>
> >
> >> Fixes: c706863bc890 ("net: ip6_gre: always reports o_key to userspace")
> >> CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >> Signed-off-by: Petr Machata <petrm@mellanox.com>
> >
> > This commit you are fixing exists in the 'net' tree, therefore this is
> > a bug fix and should be targetted at 'net'.
> 
> My mistake, I misread the follows-precedes tags at the commit.
> 
> Lorenzo, when you send your version, please send it to net.

I will do, thanks for testing.

Regards,
Lorenzo

> 
> Thanks,
> Petr