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

[Hangbin Liu <liuhangbin@gmail.com>]

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