From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Machata Subject: Re: [PATCH net v2 3/4] vxlan: changelink: Fix handling of default remotes Date: Mon, 17 Dec 2018 23:44:48 +0000 Message-ID: References: <0875afa5bd3c6df61c02595d9ee7a1651e9b54c5.1545068986.git.petrm@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: "netdev@vger.kernel.org" , "davem@davemloft.net" , Ido Schimmel To: Roopa Prabhu Return-path: Received: from mail-eopbgr30053.outbound.protection.outlook.com ([40.107.3.53]:41466 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726255AbeLQXpe (ORCPT ); Mon, 17 Dec 2018 18:45:34 -0500 In-Reply-To: (Roopa Prabhu's message of "Mon, 17 Dec 2018 11:59:31 -0800") Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Roopa Prabhu writes: > On Mon, Dec 17, 2018 at 10:10 AM Petr Machata 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 orderi= ng during netdev create") >> Signed-off-by: Petr Machata >> --- > > Acked-by: Roopa Prabhu > > 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.