[PATCH net-next] geneve: fix ttl inherit type

[PATCH net-next] geneve: fix ttl inherit type

[Hangbin Liu]

Phil pointed out that there is a mismatch between vxlan and geneve ttl
inherit. We should define it as a flag and use nla_put_flag to export this
opiton.

Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/geneve.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6625fab..09ab2fd 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
 	[IFLA_GENEVE_UDP_CSUM]		= { .type = NLA_U8 },
 	[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
 	[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
-	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_U8 },
+	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_FLAG },
 };
 
 static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
 		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
 		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
 		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
-		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
+		nla_total_size(0) + 	/* IFLA_GENEVE_TTL_INHERIT */
 		0;
 }
 
@@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto nla_put_failure;
 #endif
 
-	if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
+	if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
 		goto nla_put_failure;
 
 	return 0;
-- 
2.5.5

Re: [PATCH net-next] geneve: fix ttl inherit type

[Phil Sutter]

On Fri, Sep 28, 2018 at 09:09:58AM +0800, Hangbin Liu wrote:
> Phil pointed out that there is a mismatch between vxlan and geneve ttl
> inherit. We should define it as a flag and use nla_put_flag to export this
> opiton.

Same typo here. :)

Apart from that, LGTM!

Thanks, Phil

Re: [PATCH net-next] geneve: fix ttl inherit type

[David Ahern]

On 9/27/18 7:09 PM, Hangbin Liu wrote:
> Phil pointed out that there is a mismatch between vxlan and geneve ttl
> inherit. We should define it as a flag and use nla_put_flag to export this
> opiton.
> 
> Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")

same here .. getting an unknown commit id.


> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/geneve.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 6625fab..09ab2fd 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
>  	[IFLA_GENEVE_UDP_CSUM]		= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
> -	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_U8 },
> +	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_FLAG },
>  };
>  
>  static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
> -		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
> +		nla_total_size(0) + 	/* IFLA_GENEVE_TTL_INHERIT */
>  		0;
>  }
>  
> @@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		goto nla_put_failure;
>  #endif
>  
> -	if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
> +	if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
>  		goto nla_put_failure;
>  
>  	return 0;
> 

Re: [PATCH net-next] geneve: fix ttl inherit type

[Michal Kubecek]

On Fri, Sep 28, 2018 at 09:09:58AM +0800, Hangbin Liu wrote:
> Phil pointed out that there is a mismatch between vxlan and geneve ttl
> inherit. We should define it as a flag and use nla_put_flag to export this
> opiton.
> 
> Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/geneve.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 6625fab..09ab2fd 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
>  	[IFLA_GENEVE_UDP_CSUM]		= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
>  	[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
> -	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_U8 },
> +	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_FLAG },
>  };
>  
>  static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
>  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
> -		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
> +		nla_total_size(0) + 	/* IFLA_GENEVE_TTL_INHERIT */
>  		0;
>  }
>  
> @@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  		goto nla_put_failure;
>  #endif
>  
> -	if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
> +	if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
>  		goto nla_put_failure;
>  
>  	return 0;

Is it desirable to switch to a flag? If I read geneve_changelink() and
geneve_nl2info() correctly, it allows you to set the ttl_inherit flag
for an existing device but doesn't allow you to clear it. With NLA_U8,
you could distinguish three cases: set the flag (non-zero value), clear
the flag (zero value) and preserve current state (attribute not
present).

The same problem exists for vxlan but vxlan code intentionally disallows
changing the flag value for an existing device (I'm not sure if it's
because it's really impossible or just due to limits of the interface).
Unfortunately it has been already released with NLA_FLAG in 4.18,
AFAICS, so we have to live with it. But it's not too late for geneve.

Michal Kubecek

Re: [PATCH net-next] geneve: fix ttl inherit type

[Hangbin Liu]

On Sat, Sep 29, 2018 at 01:46:19AM +0200, Michal Kubecek wrote:
> On Fri, Sep 28, 2018 at 09:09:58AM +0800, Hangbin Liu wrote:
> > Phil pointed out that there is a mismatch between vxlan and geneve ttl
> > inherit. We should define it as a flag and use nla_put_flag to export this
> > opiton.
> > 
> > Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
> > Reported-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  drivers/net/geneve.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> > index 6625fab..09ab2fd 100644
> > --- a/drivers/net/geneve.c
> > +++ b/drivers/net/geneve.c
> > @@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
> >  	[IFLA_GENEVE_UDP_CSUM]		= { .type = NLA_U8 },
> >  	[IFLA_GENEVE_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
> >  	[IFLA_GENEVE_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
> > -	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_U8 },
> > +	[IFLA_GENEVE_TTL_INHERIT]	= { .type = NLA_FLAG },
> >  };
> >  
> >  static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
> > @@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
> >  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
> >  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
> >  		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
> > -		nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
> > +		nla_total_size(0) + 	/* IFLA_GENEVE_TTL_INHERIT */
> >  		0;
> >  }
> >  
> > @@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
> >  		goto nla_put_failure;
> >  #endif
> >  
> > -	if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
> > +	if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
> >  		goto nla_put_failure;
> >  
> >  	return 0;
> 

Hi Michal,

> Is it desirable to switch to a flag? If I read geneve_changelink() and
> geneve_nl2info() correctly, it allows you to set the ttl_inherit flag
> for an existing device but doesn't allow you to clear it. With NLA_U8,
> you could distinguish three cases: set the flag (non-zero value), clear
> the flag (zero value) and preserve current state (attribute not
> present).

I re-read geneve_changelink() and I agree with you. Since we can change ttl
number, we should also be able to set/unset ttl inherit.

Phil, what do you think?

> The same problem exists for vxlan but vxlan code intentionally disallows
> changing the flag value for an existing device (I'm not sure if it's
> because it's really impossible or just due to limits of the interface).

I will re-read VXLAN RFC to confirm this.

> Unfortunately it has been already released with NLA_FLAG in 4.18,
> AFAICS, so we have to live with it. But it's not too late for geneve.

Thanks
Hangbin

Re: [PATCH net-next] geneve: fix ttl inherit type

[Hangbin Liu]

Hi David Ahern,
On Fri, Sep 28, 2018 at 11:59:37AM -0600, David Ahern wrote:
> On 9/27/18 7:09 PM, Hangbin Liu wrote:
> > Phil pointed out that there is a mismatch between vxlan and geneve ttl
> > inherit. We should define it as a flag and use nla_put_flag to export this
> > opiton.
> > 
> > Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
> 
> same here .. getting an unknown commit id.

This one targets to kernel net-next tree.

But as Michal suggested. We can leave geneve ttl inherit as NLA_U8 to
be able set/unset it. I will re-consider this patch.

Thanks
Hangbin

Re: [PATCH net-next] geneve: fix ttl inherit type

[Hangbin Liu]

On Sat, Sep 29, 2018 at 01:46:19AM +0200, Michal Kubecek wrote:
> Is it desirable to switch to a flag? If I read geneve_changelink() and
> geneve_nl2info() correctly, it allows you to set the ttl_inherit flag
> for an existing device but doesn't allow you to clear it. With NLA_U8,
> you could distinguish three cases: set the flag (non-zero value), clear
> the flag (zero value) and preserve current state (attribute not
> present).
> 
> The same problem exists for vxlan but vxlan code intentionally disallows
> changing the flag value for an existing device (I'm not sure if it's
> because it's really impossible or just due to limits of the interface).
> Unfortunately it has been already released with NLA_FLAG in 4.18,
> AFAICS, so we have to live with it. But it's not too late for geneve.
> 
> Michal Kubecek

Hi michal,

I thought about the vxlan issue and agree with you. TTL inherit is a way
to define the ttl number we should use. It also should be able to be changed
as the normal ttl. How about enabling clear ttl inherit flag like:

--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3303,13 +3303,11 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
        if (data[IFLA_VXLAN_TOS])
                conf->tos  = nla_get_u8(data[IFLA_VXLAN_TOS]);

-       if (data[IFLA_VXLAN_TTL])
-               conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
-
        if (data[IFLA_VXLAN_TTL_INHERIT]) {
-               if (changelink)
-                       return -EOPNOTSUPP;
                conf->flags |= VXLAN_F_TTL_INHERIT;
+       } else if (data[IFLA_VXLAN_TTL]) {
+               conf->flags &= ~VXLAN_F_TTL_INHERIT;
+               conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
        }


Before this fix, we disabled changing it after creating vxlan. And with this fix
we can set/unset it. I think this should not be a usage break. What do you think?

Thanks
Hangbin

[PATCHv2 net-next] geneve: allow to clear ttl inherit

[Hangbin Liu]

As Michal remaind, we should allow to clear ttl inherit. Then we will
have three states:

1. set the flag, and do ttl inherit.
2. do not set the flag, use configured ttl value, or default ttl (0) if
   not set.
3. disable ttl inherit, use previous configured ttl value, or default ttl (0).

Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
CC: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/geneve.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6625fab..82eccc9 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1325,11 +1325,15 @@ static int geneve_nl2info(struct nlattr *tb[], struct nlattr *data[],
 		info->key.tun_id = tunid;
 	}
 
-	if (data[IFLA_GENEVE_TTL])
+	if (data[IFLA_GENEVE_TTL_INHERIT]) {
+		if (nla_get_u8(data[IFLA_GENEVE_TTL_INHERIT]))
+			*ttl_inherit = true;
+		else
+			*ttl_inherit = false;
+	} else if (data[IFLA_GENEVE_TTL]) {
 		info->key.ttl = nla_get_u8(data[IFLA_GENEVE_TTL]);
-
-	if (data[IFLA_GENEVE_TTL_INHERIT])
-		*ttl_inherit = true;
+		*ttl_inherit = false;
+	}
 
 	if (data[IFLA_GENEVE_TOS])
 		info->key.tos = nla_get_u8(data[IFLA_GENEVE_TOS]);
-- 
2.5.5

Re: [PATCH net-next] geneve: fix ttl inherit type

[Phil Sutter]

Hangbin,

On Sat, Sep 29, 2018 at 05:16:05PM +0800, Hangbin Liu wrote:
[...]
> > Is it desirable to switch to a flag? If I read geneve_changelink() and
> > geneve_nl2info() correctly, it allows you to set the ttl_inherit flag
> > for an existing device but doesn't allow you to clear it. With NLA_U8,
> > you could distinguish three cases: set the flag (non-zero value), clear
> > the flag (zero value) and preserve current state (attribute not
> > present).
> 
> I re-read geneve_changelink() and I agree with you. Since we can change ttl
> number, we should also be able to set/unset ttl inherit.
> 
> Phil, what do you think?

All fine with me. I'm not familiar with either of VXLAN or Geneve,
you're the experts here. I was just the random guy wondering why things
are done one way and not the other. :)

Thanks for your diligent efforts at clearing up the mysteries!

Cheers, Phil

Re: [PATCHv2 net-next] geneve: allow to clear ttl inherit

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Sat, 29 Sep 2018 23:06:29 +0800

> As Michal remaind, we should allow to clear ttl inherit. Then we will
> have three states:
> 
> 1. set the flag, and do ttl inherit.
> 2. do not set the flag, use configured ttl value, or default ttl (0) if
>    not set.
> 3. disable ttl inherit, use previous configured ttl value, or default ttl (0).
> 
> Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
> CC: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied.