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: <CAJieiUisxU2G0PdVGA+u7i6EBrMZarVd2oObaE_m0mVUbnmDoQ@mail.gmail.com>
Date:   Mon, 17 Dec 2018 11:59:31 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     Petr Machata <petrm@...lanox.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

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