From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Kubecek Subject: Re: [PATCH net-next] geneve: fix ttl inherit type Date: Sat, 29 Sep 2018 01:46:19 +0200 Message-ID: <20180928234619.GA17323@unicorn.suse.cz> References: <1538096998-20937-1-git-send-email-liuhangbin@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, David Miller , Stephen Hemminger , David Ahern , Phil Sutter To: Hangbin Liu Return-path: Received: from mx2.suse.de ([195.135.220.15]:50234 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726746AbeI2GM2 (ORCPT ); Sat, 29 Sep 2018 02:12:28 -0400 Content-Disposition: inline In-Reply-To: <1538096998-20937-1-git-send-email-liuhangbin@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 > Signed-off-by: Hangbin Liu > --- > 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