netdev - Re: [PATCH net v2 3/4] vxlan: changelink: Fix handling of default remotes

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: <wiha7l3afbv.fsf@dev-r-vrt-156.mtr.labs.mlnx>
Date:   Mon, 17 Dec 2018 23:44:48 +0000
From:   Petr Machata <petrm@...lanox.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net v2 3/4] vxlan: changelink: Fix handling of default
 remotes

Roopa Prabhu <roopa@...ulusnetworks.com> writes:

> On Mon, Dec 17, 2018 at 10:10 AM Petr Machata <petrm@...lanox.com> wrote:
>>
>> Default remotes are stored as FDB entries with an Ethernet address of
>> 00:00:00:00:00:00. When a request is made to change a remote address of
>> a VXLAN device, vxlan_changelink() first deletes the existing default
>> remote, and then creates a new FDB entry.
>>
>> This works well as long as the list of default remotes matches exactly
>> the configuration of a VXLAN remote address. Thus when the VXLAN device
>> has a remote of X, there should be exactly one default remote FDB entry
>> X. If the VXLAN device has no remote address, there should be no such
>> entry.
>>
>> Besides using "ip link set", it is possible to manipulate the list of
>> default remotes by using the "bridge fdb". It is therefore easy to break
>> the above condition. Under such circumstances, the __vxlan_fdb_delete()
>> call doesn't delete the FDB entry itself, but just one remote. The
>> following vxlan_fdb_create() then creates a new FDB entry, leading to a
>> situation where two entries exist for the address 00:00:00:00:00:00,
>> each with a different subset of default remotes.
>>
>> An even more obvious breakage rooted in the same cause can be observed
>> when a remote address is configured for a VXLAN device that did not have
>> one before. In that case vxlan_changelink() doesn't remove any remote,
>> and just creates a new FDB entry for the new address:
>>
>> $ ip link add name vx up type vxlan id 2000 dstport 4789
>> $ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.20 self permanent
>> $ bridge fdb ap dev vx 00:00:00:00:00:00 dst 192.0.2.30 self permanent
>> $ ip link set dev vx type vxlan remote 192.0.2.30
>> $ bridge fdb sh dev vx | grep 00:00:00:00:00:00
>> 00:00:00:00:00:00 dst 192.0.2.30 self permanent <- new entry, 1 rdst
>> 00:00:00:00:00:00 dst 192.0.2.20 self permanent <- orig. entry, 2 rdsts
>> 00:00:00:00:00:00 dst 192.0.2.30 self permanent
>>
>> To fix this, instead of calling vxlan_fdb_create() directly, defer to
>> vxlan_fdb_update(). That has logic to handle the duplicates properly.
>> Additionally, it also handles notifications, so drop that call from
>> changelink as well.
>>
>> Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
>> Signed-off-by: Petr Machata <petrm@...lanox.com>
>> ---
>
> Acked-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>
> This fix looks fine. But it almost seems like the changelink Fixes tag
> is more appropriate here unless you are
> sure that it worked correctly after the changelink.

I don't follow. What do you mean?

> Though I agree
> this needs to be fixed, deployments either
> configure the default remote dst on the vxlan device or manage default
> remote dsts via fdb api in control plane
>  not both. The trigger for this bug seems like the use of both.

Yep, you need to do both to trigger this. Which is why I think this went
undetected--otherwise people would have noticed some addresses are not
flooded.