Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[Alexis Bauvin <abauvin@scaleway.com>]

Le 14 nov. 2018 à 20:58, David Ahern <dsa@cumulusnetworks.com> a écrit :
> 
> you are making this more specific than it needs to be ....
> 
> On 11/14/18 1:31 AM, Alexis Bauvin wrote:
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 27bd586b94b0..7477b5510a04 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -208,11 +208,23 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>> 	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>> }
>> 
>> +static int vxlan_get_l3mdev(struct net *net, int ifindex)
>> +{
>> +	struct net_device *dev;
>> +
>> +	dev = __dev_get_by_index(net, ifindex);
>> +	while (dev && !netif_is_l3_master(dev))
>> +		dev = netdev_master_upper_dev_get(dev);
>> +
>> +	return dev ? dev->ifindex : 0;
>> +}
> 
> l3mdev_master_ifindex_by_index should work instead of defining this for
> vxlan.
> 
> But I do not believe you need this function.

l3mdev_master_ifindex_by_index does not recursively climbs up the master chain.
This means that if the l3mdev is not a direct master of the device, it will not
be found.

E.G. Calling l3mdev_master_ifindex_by_index with the index of eth0 will
return 0:

+------+     +-----+     +----------+
|      |     |     |     |          |
| eth0 +-----+ br0 +-----+ vrf-blue |
|      |     |     |     |          |
+------+     +-----+     +----------+

This is because the underlying l3mdev_master_dev_rcu function fetches the master
(br0 in this case), checks whether it is an l3mdev (which it is not), and
returns its index if so.

So if using l3mdev_master_dev_rcu, using eth0 as a lower device will still bind
to no specific device, thus in the default VRF.

Maybe I should have patched l3mdev_master_dev_rcu to do a recursive resolution
(as vxlan_get_l3mdev does), but I don’t know the impact of such a change.

>> +
>> /* Find VXLAN socket based on network namespace, address family and UDP port
>>  * and enabled unshareable flags.
>>  */
>> static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
>> -					  __be16 port, u32 flags)
>> +					  __be16 port, u32 flags,
>> +					  int l3mdev_ifindex)
>> {
>> 	struct vxlan_sock *vs;
>> 
>> @@ -221,7 +233,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
>> 	hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
>> 		if (inet_sk(vs->sock->sk)->inet_sport == port &&
>> 		    vxlan_get_sk_family(vs) == family &&
>> -		    vs->flags == flags)
>> +		    vs->flags == flags &&
>> +		    vs->sock->sk->sk_bound_dev_if == l3mdev_ifindex)
> 
> Why not allow the vxlan socket to bind to any ifindex? In that case this
> socket lookup follows what we do for tcp, udp and raw sockets, and  you
> don't need to call out vrf / l3mdev directly (ie.,
> s/l3mdev_ifindex/ifindex/g) - it comes for free.

Reword will be done in next version!