* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 1:09 [PATCH net-next] geneve: fix ttl inherit type Hangbin Liu
@ 2018-09-28 10:38 ` Phil Sutter
2018-09-28 17:59 ` David Ahern
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2018-09-28 10:38 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, David Miller, Stephen Hemminger, David Ahern
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 1:09 [PATCH net-next] geneve: fix ttl inherit type Hangbin Liu
2018-09-28 10:38 ` Phil Sutter
@ 2018-09-28 17:59 ` David Ahern
2018-09-29 9:20 ` Hangbin Liu
2018-09-28 23:46 ` Michal Kubecek
2018-09-29 15:06 ` [PATCHv2 net-next] geneve: allow to clear ttl inherit Hangbin Liu
3 siblings, 1 reply; 10+ messages in thread
From: David Ahern @ 2018-09-28 17:59 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: David Miller, Stephen Hemminger, Phil Sutter
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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 17:59 ` David Ahern
@ 2018-09-29 9:20 ` Hangbin Liu
0 siblings, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2018-09-29 9:20 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, David Miller, Stephen Hemminger, Phil Sutter
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 1:09 [PATCH net-next] geneve: fix ttl inherit type Hangbin Liu
2018-09-28 10:38 ` Phil Sutter
2018-09-28 17:59 ` David Ahern
@ 2018-09-28 23:46 ` Michal Kubecek
2018-09-29 9:16 ` Hangbin Liu
2018-09-29 15:03 ` Hangbin Liu
2018-09-29 15:06 ` [PATCHv2 net-next] geneve: allow to clear ttl inherit Hangbin Liu
3 siblings, 2 replies; 10+ messages in thread
From: Michal Kubecek @ 2018-09-28 23:46 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, David Miller, Stephen Hemminger, David Ahern, 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.
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 23:46 ` Michal Kubecek
@ 2018-09-29 9:16 ` Hangbin Liu
2018-10-01 10:56 ` Phil Sutter
2018-09-29 15:03 ` Hangbin Liu
1 sibling, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2018-09-29 9:16 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Stephen Hemminger, David Ahern, Phil Sutter
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-29 9:16 ` Hangbin Liu
@ 2018-10-01 10:56 ` Phil Sutter
0 siblings, 0 replies; 10+ messages in thread
From: Phil Sutter @ 2018-10-01 10:56 UTC (permalink / raw)
To: Hangbin Liu
Cc: Michal Kubecek, netdev, David Miller, Stephen Hemminger, David Ahern
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] geneve: fix ttl inherit type
2018-09-28 23:46 ` Michal Kubecek
2018-09-29 9:16 ` Hangbin Liu
@ 2018-09-29 15:03 ` Hangbin Liu
1 sibling, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2018-09-29 15:03 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Stephen Hemminger, David Ahern, Phil Sutter
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2 net-next] geneve: allow to clear ttl inherit
2018-09-28 1:09 [PATCH net-next] geneve: fix ttl inherit type Hangbin Liu
` (2 preceding siblings ...)
2018-09-28 23:46 ` Michal Kubecek
@ 2018-09-29 15:06 ` Hangbin Liu
2018-10-04 20:55 ` David Miller
3 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2018-09-29 15:06 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Phil Sutter, Hangbin Liu, Michal Kubecek
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCHv2 net-next] geneve: allow to clear ttl inherit
2018-09-29 15:06 ` [PATCHv2 net-next] geneve: allow to clear ttl inherit Hangbin Liu
@ 2018-10-04 20:55 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-10-04 20:55 UTC (permalink / raw)
To: liuhangbin; +Cc: netdev, phil, mkubecek
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.
^ permalink raw reply [flat|nested] 10+ messages in thread