| Message ID | 1538096998-20937-1-git-send-email-liuhangbin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] geneve: fix ttl inherit type | expand |
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
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; >
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
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
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
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
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
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;
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(-)