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

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: <D9A396BA-D092-4302-B94A-EC12885E236A@scaleway.com>
Date:   Fri, 16 Nov 2018 11:41:46 +0100
From:   Alexis Bauvin <abauvin@...leway.com>
To:     David Ahern <dsa@...ulusnetworks.com>, roopa@...ulusnetworks.com,
        nicolas.dichtel@...nd.com
Cc:     netdev@...r.kernel.org, akherbouche@...leway.com,
        Alexis Bauvin <abauvin@...leway.com>
Subject: Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

Le 16 nov. 2018 à 08:37, David Ahern <dsa@...ulusnetworks.com> a écrit :
> On 11/15/18 2:05 AM, Alexis Bauvin wrote:
>> Le 14 nov. 2018 à 20:58, David Ahern <dsa@...ulusnetworks.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?

The case I am trying to cover here is the user creating a VXLAN device with eth0
as its lower device (ip link add vxlan0 type vxlan ... dev eth0), thus ignoring
the fact that it should be br0 (the actual L3 interface). In this case, the only
information available from the module's point of view is eth0. I may be wrong,
but eth0 is indirectly "part" of vrf-blue (even if it is only L2), as packets
flowing in from it would land in vrf-blue if L3.

As for the device stacking, I am only interested in the VXLAN underlay: the
VXLAN device itself could be in a specific VRF or not, it should not influence
its underlay. 

+----------+                         +---------+
|          |                         |         |
| vrf-blue |                         | vrf-red |
|          |                         |         |
+----+-----+                         +----+----+
     |                                    |
     |                                    |
+----+-----+                         +----+----+
|          |                         |         |
| br-blue  |                         | br-red  |
|          |                         |         |
+----+-----+                         +---+-+---+
     |                                   | |
     |                             +-----+ +-----+
     |                             |             |
+----+-----+                +------+----+   +----+----+
|          |  lower device  |           |   |         |
|   eth0   | <- - - - - - - | vxlan-red |   | tap-red | (... more taps)
|          |                |           |   |         |
+----------+                +-----------+   +---------+


While I don't see any use case for having a bridged uplink when using VXLAN,
someone may and would find a different behavior depending on the lower device.
In the above example, vxlan-red's lower device should be br-blue, but a user
would expect the underlay VRF (vrf-blue) to still be taken into account if eth0
was used as the lower device.

A different approach would be to check if the lower device is a bridge. If not,
fetch a potential master bridge. Then, with this L3/router interface, we fetch
the l3mdev with l3mdev_master_ifindex_by_index (if any).

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

Ok! What is the best approach in your opinion?