netdev - Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes

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]

Date:   Thu, 29 Nov 2018 14:56:31 +0100
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more
 attributes

2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@...ulusnetworks.com>
> 
> We started very conservative when supporting changelink
> especially because not all attribute changes could be
> tested. This patch opens up a few more attributes for
> changelink. The reason for choosing this set of attributes
> is based on code references for these attributes. I have
> tested TTL changes and did some changelink api testing
> to sanity test the others.
> 
> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
> ---
>  drivers/net/vxlan.c | 36 ++++--------------------------------
>  1 file changed, 4 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 9110662..73caa65 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3438,11 +3438,8 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
>  	if (data[IFLA_VXLAN_TTL])
>  		conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
>  
> -	if (data[IFLA_VXLAN_TTL_INHERIT]) {
> -		if (changelink)
> -			return -EOPNOTSUPP;
> +	if (data[IFLA_VXLAN_TTL_INHERIT])
>  		conf->flags |= VXLAN_F_TTL_INHERIT;
> -	}

This doesn't give us an option to disable TTL_INHERIT after it was
enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.

> -	if (data[IFLA_VXLAN_GBP]) {
> -		if (changelink)
> -			return -EOPNOTSUPP;
> +	if (data[IFLA_VXLAN_GBP])
>  		conf->flags |= VXLAN_F_GBP;
> -	}
>  
> -	if (data[IFLA_VXLAN_GPE]) {
> -		if (changelink)
> -			return -EOPNOTSUPP;
> +	if (data[IFLA_VXLAN_GPE])
>  		conf->flags |= VXLAN_F_GPE;
> -	}

GPE implies running a different setup function (vxlan_raw_setup() vs
vxlan_ether_setup()), that vxlan_config_apply() only calls for
!changelink. I think this is incomplete.

I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
same socket, I'm not sure how that would work. Normally, they each try
to create a separate socket, and pass the GPE flag on to the
associated vxlan_sock. I suspect that's also a problem with rx
offload.

> -	if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> -		if (changelink)
> -			return -EOPNOTSUPP;
> +	if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
>  		conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> -	}
>  
>  	if (tb[IFLA_MTU]) {
>  		if (changelink)
> -- 
> 2.1.4
> 

-- 
Sabrina