| Message ID | 1510563806-8012-1-git-send-email-liuhangbin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] geneve: show remote address and checksum info even after link down | expand |
On Mon, Nov 13, 2017 at 05:03:26PM +0800, Hangbin Liu wrote: > geneve->sock4/6 were added with geneve_open and released with geneve_stop. > So when geneve link down, we will not able to show remote address and > checksum info after commit 11387fe4a98 ("geneve: fix fill_info when using > collect_metadata"). > > Fix this by reset back to the previous behavior: only show the corresponding > remote address and always show checksum info. Thanks Hangbin, I think we can do better though and maybe avoid another round of this churn. > > Fixes: 11387fe4a98 ("geneve: fix fill_info when using collect_metadata") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/geneve.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index ed51018..b010db7 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -1511,32 +1511,18 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) > if (nla_put_u32(skb, IFLA_GENEVE_ID, vni)) > goto nla_put_failure; > > - if (rtnl_dereference(geneve->sock4)) { > + if (ip_tunnel_info_af(info) == AF_INET) { > if (nla_put_in_addr(skb, IFLA_GENEVE_REMOTE, > info->key.u.ipv4.dst)) > goto nla_put_failure; > We can avoid passing up *_REMOTE{,6} for COLLECT_METADATA. They're mutually exclusive. > - if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, > - !!(info->key.tun_flags & TUNNEL_CSUM))) > - goto nla_put_failure; > - > - } > - > #if IS_ENABLED(CONFIG_IPV6) > - if (rtnl_dereference(geneve->sock6)) { > + } else { > if (nla_put_in6_addr(skb, IFLA_GENEVE_REMOTE6, > &info->key.u.ipv6.dst)) > goto nla_put_failure; > - > - if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX, > - !(info->key.tun_flags & TUNNEL_CSUM))) > - goto nla_put_failure; > - > - if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX, > - !geneve->use_udp6_rx_checksums)) > - goto nla_put_failure; > - } > #endif > + } > > if (nla_put_u8(skb, IFLA_GENEVE_TTL, info->key.ttl) || > nla_put_u8(skb, IFLA_GENEVE_TOS, info->key.tos) || > @@ -1550,6 +1536,14 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) > if (nla_put_flag(skb, IFLA_GENEVE_COLLECT_METADATA)) > goto nla_put_failure; > } > + > + if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, > + !!(info->key.tun_flags & TUNNEL_CSUM)) || > + nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX, > + !(info->key.tun_flags & TUNNEL_CSUM)) || These two are nonsensical for COLLECT_METADATA as they come from the skb tun_info. They can be moved to inside the ipv4/ipv6 checks. For non-metadata, it doesn't make sense to pass them both as the tunnel is either ipv4 or ipv6, but not both. > + nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX, > + !geneve->use_udp6_rx_checksums)) > + goto nla_put_failure; > return 0; > > nla_put_failure:
Hi Eric, Thanks for the comments. On Mon, Nov 13, 2017 at 03:27:25PM -0500, Eric Garver wrote: > > Fixes: 11387fe4a98 ("geneve: fix fill_info when using collect_metadata") > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > drivers/net/geneve.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > index ed51018..b010db7 100644 > > --- a/drivers/net/geneve.c > > +++ b/drivers/net/geneve.c > > @@ -1511,32 +1511,18 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) > > if (nla_put_u32(skb, IFLA_GENEVE_ID, vni)) > > goto nla_put_failure; > > > > - if (rtnl_dereference(geneve->sock4)) { > > + if (ip_tunnel_info_af(info) == AF_INET) { > > if (nla_put_in_addr(skb, IFLA_GENEVE_REMOTE, > > info->key.u.ipv4.dst)) > > goto nla_put_failure; > > > > We can avoid passing up *_REMOTE{,6} for COLLECT_METADATA. They're > mutually exclusive. Do you mean something like + bool metadata = geneve->collect_md; - if (rtnl_dereference(geneve->sock4)) { + if (!metadata && ip_tunnel_info_af(info) == AF_INET) { > > > - if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, > > - !!(info->key.tun_flags & TUNNEL_CSUM))) > > - goto nla_put_failure; > > - > > - } > > - > > #if IS_ENABLED(CONFIG_IPV6) > > - if (rtnl_dereference(geneve->sock6)) { > > + } else { and - if (rtnl_dereference(geneve->sock6)) { + } else if (!metadata) { ? > > + > > + if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, > > + !!(info->key.tun_flags & TUNNEL_CSUM)) || > > + nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX, > > + !(info->key.tun_flags & TUNNEL_CSUM)) || > > These two are nonsensical for COLLECT_METADATA as they come from the skb > tun_info. They can be moved to inside the ipv4/ipv6 checks. For > non-metadata, it doesn't make sense to pass them both as the tunnel is > either ipv4 or ipv6, but not both. OK, I will put them back to the ipv4/6 chunks. Thanks Hangbin
On Tue, Nov 14, 2017 at 10:39:41AM +0800, Hangbin Liu wrote: > Hi Eric, > > Thanks for the comments. > > On Mon, Nov 13, 2017 at 03:27:25PM -0500, Eric Garver wrote: > > > Fixes: 11387fe4a98 ("geneve: fix fill_info when using collect_metadata") > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > > --- > > > drivers/net/geneve.c | 28 +++++++++++----------------- > > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > > index ed51018..b010db7 100644 > > > --- a/drivers/net/geneve.c > > > +++ b/drivers/net/geneve.c > > > @@ -1511,32 +1511,18 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) > > > if (nla_put_u32(skb, IFLA_GENEVE_ID, vni)) > > > goto nla_put_failure; > > > > > > - if (rtnl_dereference(geneve->sock4)) { > > > + if (ip_tunnel_info_af(info) == AF_INET) { > > > if (nla_put_in_addr(skb, IFLA_GENEVE_REMOTE, > > > info->key.u.ipv4.dst)) > > > goto nla_put_failure; > > > > > > > We can avoid passing up *_REMOTE{,6} for COLLECT_METADATA. They're > > mutually exclusive. > > Do you mean something like > > + bool metadata = geneve->collect_md; > - if (rtnl_dereference(geneve->sock4)) { > + if (!metadata && ip_tunnel_info_af(info) == AF_INET) { Yes. > > > > > - if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, > > > - !!(info->key.tun_flags & TUNNEL_CSUM))) > > > - goto nla_put_failure; > > > - > > > - } > > > - > > > #if IS_ENABLED(CONFIG_IPV6) > > > - if (rtnl_dereference(geneve->sock6)) { > > > + } else { > > and > > - if (rtnl_dereference(geneve->sock6)) { > + } else if (!metadata) { > > ? Yes.
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index ed51018..b010db7 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -1511,32 +1511,18 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) if (nla_put_u32(skb, IFLA_GENEVE_ID, vni)) goto nla_put_failure; - if (rtnl_dereference(geneve->sock4)) { + if (ip_tunnel_info_af(info) == AF_INET) { if (nla_put_in_addr(skb, IFLA_GENEVE_REMOTE, info->key.u.ipv4.dst)) goto nla_put_failure; - if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, - !!(info->key.tun_flags & TUNNEL_CSUM))) - goto nla_put_failure; - - } - #if IS_ENABLED(CONFIG_IPV6) - if (rtnl_dereference(geneve->sock6)) { + } else { if (nla_put_in6_addr(skb, IFLA_GENEVE_REMOTE6, &info->key.u.ipv6.dst)) goto nla_put_failure; - - if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX, - !(info->key.tun_flags & TUNNEL_CSUM))) - goto nla_put_failure; - - if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX, - !geneve->use_udp6_rx_checksums)) - goto nla_put_failure; - } #endif + } if (nla_put_u8(skb, IFLA_GENEVE_TTL, info->key.ttl) || nla_put_u8(skb, IFLA_GENEVE_TOS, info->key.tos) || @@ -1550,6 +1536,14 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev) if (nla_put_flag(skb, IFLA_GENEVE_COLLECT_METADATA)) goto nla_put_failure; } + + if (nla_put_u8(skb, IFLA_GENEVE_UDP_CSUM, + !!(info->key.tun_flags & TUNNEL_CSUM)) || + nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX, + !(info->key.tun_flags & TUNNEL_CSUM)) || + nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX, + !geneve->use_udp6_rx_checksums)) + goto nla_put_failure; return 0; nla_put_failure:
geneve->sock4/6 were added with geneve_open and released with geneve_stop. So when geneve link down, we will not able to show remote address and checksum info after commit 11387fe4a98 ("geneve: fix fill_info when using collect_metadata"). Fix this by reset back to the previous behavior: only show the corresponding remote address and always show checksum info. Fixes: 11387fe4a98 ("geneve: fix fill_info when using collect_metadata") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- drivers/net/geneve.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)