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

[David Ahern <dsa@cumulusnetworks.com>]

On 11/15/18 2:05 AM, Alexis Bauvin wrote:
> 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 |
> |      |     |     |     |          |
> +------+     +-----+     +----------+
> 

eth0 is not the L3/router interface in this picture; br0 is. There
should not be a need for invoking l3mdev_master_ifindex_by_index on eth0.

What device stacking are you expecting to handle with vxlan devices?
vxlan on eth0 with vxlan devices in a VRF? vxlan devices into a bridge
with the bridge (or SVI) enslaved to a VRF?


> 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.

no, that is definitely the wrong the approach.