netdev - Re: [PATCH net-next 3/3] vxlan: allow multiple VXLANs with same VNI for IPv6 link-local addresses

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:   Wed, 15 Mar 2017 15:29:29 +0100
From:   Matthias Schiffer <mschiffer@...verse-factory.net>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     davem@...emloft.net, hannes@...essinduktion.org, pshelar@....org,
        aduyck@...antis.com, roopa@...ulusnetworks.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/3] vxlan: allow multiple VXLANs with same VNI
 for IPv6 link-local addresses

On 03/14/2017 04:28 PM, Jiri Benc wrote:
> On Fri, 10 Mar 2017 23:39:44 +0100, Matthias Schiffer wrote:
>> @@ -233,17 +234,30 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, __be32 vni)
>>  		vni = 0;
>>  
>>  	hlist_for_each_entry_rcu(vxlan, vni_head(vs, vni), hlist) {
>> -		if (vxlan->default_dst.remote_vni == vni)
>> -			return vxlan;
>> +		if (vxlan->default_dst.remote_vni != vni)
>> +			continue;
>> +
>> +		if (IS_ENABLED(CONFIG_IPV6)) {
>> +			const struct vxlan_config *cfg = &vxlan->cfg;
>> +
>> +			if (cfg->remote_ifindex != 0 &&
>> +			    cfg->remote_ifindex != ifindex &&
>> +			    cfg->saddr.sa.sa_family == AF_INET6 &&
>> +			    (ipv6_addr_type(&cfg->saddr.sin6.sin6_addr) &
>> +			     IPV6_ADDR_LINKLOCAL))
> 
> Calculating this (especially ipv6_addr_type) on every received packet
> looks unnecessarily expensive. Just store the fact the the local
> address is link-local in a flag during config. And compare the flag
> first before considering remote_ifindex.
> 
> This is especially important for lwtunnels which can have anything in
> the saddr and remote_ifindex, yet those fields are ignored and the
> vni 0 entry has to be returned. It also means that the link-local flag
> must not be set for lwtunnels.
> 
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +		if (conf->remote_ifindex != tmp->cfg.remote_ifindex &&
>> +		    conf->saddr.sa.sa_family == AF_INET6 &&
>> +		    tmp->cfg.saddr.sa.sa_family == AF_INET6 &&
>> +		    (ipv6_addr_type(&conf->saddr.sin6.sin6_addr) &
>> +		     IPV6_ADDR_LINKLOCAL) &&
>> +		    (ipv6_addr_type(&tmp->cfg.saddr.sin6.sin6_addr) &
>> +		     IPV6_ADDR_LINKLOCAL))
>> +			continue;
>> +#endif
> 
> In patch 1, you're checking for either source and destination address
> being link-local, while here you're consider only those that have both
> addresses link-local.
> 
> Wouldn't it be better to plainly reject configuration that has one
> address link-local but not the other one?

While ensuring that the destination address is link-local iff the source
address is would also be an option, it didn't seem too useful as the
destination address will be a multicast address anyways in "normal" VXLAN
configurations. If we really want to check this, I guess the valid
combinations are:

source link-local - destination link-local UC
source link-local - destination link-local MC
source global/... - destination global/... UC
source global/... - destination any MC

Does this make sense?


> 
>  Jiri

Thanks for your comments, I'll send a v2 soon.

Matthias






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)