netdev - Re: [PATCH net] geneve: show remote address and checksum info even after link down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171113202724.GN25853@dev-rhel7>
Date:   Mon, 13 Nov 2017 15:27:25 -0500
From:   Eric Garver <e@...g.me>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Pravin B Shelar <pshelar@....org>,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH net] geneve: show remote address and checksum info even
 after link down

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@...il.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: