netdev - Re: [RFC v3 3/3] vxlan: handle underlay VRF changes

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:   Tue, 20 Nov 2018 17:58:23 +0100
From:   Alexis Bauvin <abauvin@...leway.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>,
        David Ahern <dsa@...ulusnetworks.com>
Cc:     netdev <netdev@...r.kernel.org>, akherbouche@...leway.com
Subject: Re: [RFC v3 3/3] vxlan: handle underlay VRF changes

Le 20 nov. 2018 à 16:35, Roopa Prabhu <roopa@...ulusnetworks.com> a écrit :
> 
> On Tue, Nov 20, 2018 at 7:04 AM David Ahern <dsa@...ulusnetworks.com> wrote:
>> 
>> On 11/20/18 7:23 AM, Alexis Bauvin wrote:
>>> When underlay VRF changes, either because the lower device itself changed,
>>> or its VRF changed, this patch releases the current socket of the VXLAN
>>> device and recreates another one in the right VRF. This allows for
>>> on-the-fly change of the underlay VRF of a VXLAN device.
>>> 
>>> Signed-off-by: Alexis Bauvin <abauvin@...leway.com>
>>> Reviewed-by: Amine Kherbouche <akherbouche@...leway.com>
>>> Tested-by: Amine Kherbouche <akherbouche@...leway.com>
>>> ---
>>> drivers/net/vxlan.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 94 insertions(+)
>>> 
>>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>>> index a3de08122269..1e6ccad6df6a 100644
>>> --- a/drivers/net/vxlan.c
>>> +++ b/drivers/net/vxlan.c
>>> @@ -208,6 +208,18 @@ 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_is_in_l3mdev_chain(struct net_device *chain,
>>> +                                 struct net_device *dev)
>>> +{
>>> +     if (!chain)
>>> +             return 0;
>>> +
>>> +     if (chain->ifindex == dev->ifindex)
>>> +             return 1;
>>> +     return vxlan_is_in_l3mdev_chain(netdev_master_upper_dev_get(chain),
>>> +                                     dev);
>>> +}
>> 
>> This should return bool and true/false.
>> 
>> Also, why l3mdev in the name? None of the checks look at whether it is
>> an l3mdev master.
>> 
>> And again here, someone more familiar with the vxlan code should review it.
>> 
> 
> 
> I understand the need for patch 2. But I don't understand the need for
> the complexity this patch introduces (especially implicit down and up
> of the vxlan device).
> Alexis, If your underlay routing changes,  you can down and up the
> vxlan device from user-space correct ?. This should be true for any
> tunnel device.

Yes. Without this patch, down-up the vxlan interface is enough to handle
the underlay VRF change. This patch is more a convenience than a real
need.

Furthermore, an issue with automatic down/up of the interface is a silent
fail when mixing underlays in the default vrf and in specific ones. Say
you are in the following situation:

       +----------+
       |          |
       | vrf-blue |
       |          |
       +--+----+--+
          |    |
     +----+    +----+
     |              |
+----+----+    +----+----+
|         |    |         |
| dummy-a |    | dummy-b |
|         |    |         |
+---------+    +---------+
     .              .
     .(lower device).
     .              .
+---------+    +---------+
|         |    |         |
| vxlan-a |    | vxlan-b |
|         |    |         |
+---------+    +---------+

A socket bound to vrf-blue listens on *:4789, thus owning the port. If moving an
underlay to the default vrf (ip link set dummy-b nomaster), a new socket will be
created, unbound to any interface and listening on *:4789. However, because it
will be in the default vrf, it will try to take ownership of port 4789 on ALL
vrfs, and fail because this port is already owned in vrf-blue for vxlan-a.

At least, a manual down/up of the vxlan interface would fail when upping the
interface, and spit out "Address already in use" through netlink to the
responsible ip link set up.

Dropping this patch would be sensible given its implicit actions, or fixing it
to make the vxlan interface really down when such a situation happens.

It may not be the right place to ask, but I don’t know the reason behind this
default vrf behaviour.