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

From: Michal Kubecek <mkubecek@suse.cz>
To: Hangbin Liu <liuhangbin@gmail.com>
Cc: netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
	Stephen Hemminger <stephen@networkplumber.org>,
	David Ahern <dsahern@gmail.com>, Phil Sutter <phil@nwl.cc>
Subject: Re: [PATCH net-next] geneve: fix ttl inherit type
Date: Sat, 29 Sep 2018 01:46:19 +0200	[thread overview]
Message-ID: <20180928234619.GA17323@unicorn.suse.cz> (raw)
In-Reply-To: <1538096998-20937-1-git-send-email-liuhangbin@gmail.com>

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