[PATCH net v3 0/4] vxlan: Various fixes

[PATCH net v3 0/4] vxlan: Various fixes

[Petr Machata]

This patch set contains three fixes for the vxlan driver.

Patch #1 fixes handling of offload mark on replaced VXLAN FDB entries. A
way to trigger this is to replace the FDB entry with one that can not be
offloaded. A future patch set should make it possible to veto such FDB
changes. However the FDB might still fail to be offloaded due to another
issue, and the offload mark should reflect that.

Patch #2 fixes problems in __vxlan_dev_create() when a call to
rtnl_configure_link() fails. These failures would be tricky to hit on a
real system, the most likely vector is through an error in vxlan_open().
However, with the abovementioned vetoing patchset, vetoing the created
entry would trigger the same problems (and be easier to reproduce).

Patch #3 fixes a problem in vxlan_changelink(). In situations where the
default remote configured in the FDB table (if any) does not exactly
match the remote address configured at the VXLAN device, changing the
remote address breaks the default FDB entry. Patch #4 is then a self
test for this issue.

v3:
- Patch #2:
    - Reuse the same errout block for both cleanup paths. Use a bool to
      decide whether the unregister_netdevice() call should be made.

v2:
- Drop former patch #3
- Patch #2:
    - Delete the default entry before calling unregister_netdevice(). That
      takes care of former patch #3, hence tweak the commit message to
      mention that problem as well.

Petr Machata (4):
  vxlan: Unmark offloaded bit on replaced FDB entries
  vxlan: Fix error path in __vxlan_dev_create()
  vxlan: changelink: Fix handling of default remotes
  selftests: net: Add test_vxlan_fdb_changelink.sh

 drivers/net/vxlan.c                                | 21 ++++++++++------
 tools/testing/selftests/net/Makefile               |  1 +
 .../selftests/net/test_vxlan_fdb_changelink.sh     | 29 ++++++++++++++++++++++
 3 files changed, 44 insertions(+), 7 deletions(-)
 create mode 100755 tools/testing/selftests/net/test_vxlan_fdb_changelink.sh

-- 
2.4.11

[PATCH net v3 1/4] vxlan: Unmark offloaded bit on replaced FDB entries

[Petr Machata]

When rdst of an offloaded FDB entry is replaced, it certainly isn't
offloaded anymore. Drivers are notified about such replacements, and can
re-mark the entry as offloaded again if they so wish. However until a
driver does so explicitly, assume a replaced FDB entry is not offloaded.

Note that replaces coming via vxlan_fdb_external_learn_add() are always
immediately followed by an explicit offload marking.

Fixes: 0efe11733356 ("vxlan: Support marking RDSTs as offloaded")
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/vxlan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 297cdeaef479..c9956c08edf5 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -568,6 +568,7 @@ static int vxlan_fdb_replace(struct vxlan_fdb *f,
 	rd->remote_port = port;
 	rd->remote_vni = vni;
 	rd->remote_ifindex = ifindex;
+	rd->offloaded = false;
 	return 1;
 }
 
-- 
2.4.11

[PATCH net v3 2/4] vxlan: Fix error path in __vxlan_dev_create()

[Petr Machata]

When a failure occurs in rtnl_configure_link(), the current code
calls unregister_netdevice() to roll back the earlier call to
register_netdevice(), and jumps to errout, which calls
vxlan_fdb_destroy().

However unregister_netdevice() calls transitively ndo_uninit, which is
vxlan_uninit(), and that already takes care of deleting the default FDB
entry by calling vxlan_fdb_delete_default(). Since the entry added
earlier in __vxlan_dev_create() is exactly the default entry, the
cleanup code in the errout block always leads to double free and thus a
panic.

Besides, since vxlan_fdb_delete_default() always destroys the FDB entry
with notification enabled, the deletion of the default entry is notified
even before the addition was notified.

Instead, move the unregister_netdevice() call after the manual destroy,
which solves both problems.

Fixes: 0241b836732f ("vxlan: fix default fdb entry netlink notify ordering during netdev create")
Signed-off-by: Petr Machata <petrm@mellanox.com>
---

Notes:
    v3:
    - Reuse the same errout block for both cleanup paths. Use a bool to
      decide whether the unregister_netdevice() call should be made.
    
    v2:
    - Delete the default entry before calling unregister_netdevice(). That
      takes care of former patch #3, hence tweak the commit message to
      mention that problem as well.

 drivers/net/vxlan.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index c9956c08edf5..82e78d6fd9ae 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3259,6 +3259,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_fdb *f = NULL;
+	bool unregister = false;
 	int err;
 
 	err = vxlan_dev_configure(net, dev, conf, false, extack);
@@ -3284,12 +3285,11 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 	err = register_netdevice(dev);
 	if (err)
 		goto errout;
+	unregister = true;
 
 	err = rtnl_configure_link(dev, NULL);
-	if (err) {
-		unregister_netdevice(dev);
+	if (err)
 		goto errout;
-	}
 
 	/* notify default fdb entry */
 	if (f)
@@ -3297,9 +3297,16 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
 
 	list_add(&vxlan->next, &vn->vxlan_list);
 	return 0;
+
 errout:
+	/* unregister_netdevice() destroys the default FDB entry with deletion
+	 * notification. But the addition notification was not sent yet, so
+	 * destroy the entry by hand here.
+	 */
 	if (f)
 		vxlan_fdb_destroy(vxlan, f, false);
+	if (unregister)
+		unregister_netdevice(dev);
 	return err;
 }
 
-- 
2.4.11

[PATCH net v3 3/4] vxlan: changelink: Fix handling of default remotes

[Petr Machata]

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@mellanox.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 drivers/net/vxlan.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 82e78d6fd9ae..0565f8880199 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3542,7 +3542,6 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
 	struct vxlan_rdst *dst = &vxlan->default_dst;
 	struct vxlan_rdst old_dst;
 	struct vxlan_config conf;
-	struct vxlan_fdb *f = NULL;
 	int err;
 
 	err = vxlan_nl2conf(tb, data,
@@ -3568,19 +3567,19 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
 					   old_dst.remote_ifindex, 0);
 
 		if (!vxlan_addr_any(&dst->remote_ip)) {
-			err = vxlan_fdb_create(vxlan, all_zeros_mac,
+			err = vxlan_fdb_update(vxlan, all_zeros_mac,
 					       &dst->remote_ip,
 					       NUD_REACHABLE | NUD_PERMANENT,
+					       NLM_F_APPEND | NLM_F_CREATE,
 					       vxlan->cfg.dst_port,
 					       dst->remote_vni,
 					       dst->remote_vni,
 					       dst->remote_ifindex,
-					       NTF_SELF, &f);
+					       NTF_SELF);
 			if (err) {
 				spin_unlock_bh(&vxlan->hash_lock);
 				return err;
 			}
-			vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
 		}
 		spin_unlock_bh(&vxlan->hash_lock);
 	}
-- 
2.4.11

[PATCH net v3 4/4] selftests: net: Add test_vxlan_fdb_changelink.sh

[Petr Machata]

Add a test to exercise the fix from the previous patch.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 tools/testing/selftests/net/Makefile               |  1 +
 .../selftests/net/test_vxlan_fdb_changelink.sh     | 29 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100755 tools/testing/selftests/net/test_vxlan_fdb_changelink.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 256d82d5fa87..923570a9708a 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -7,6 +7,7 @@ CFLAGS += -I../../../../usr/include/
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
 TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh
 TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
+TEST_PROGS += test_vxlan_fdb_changelink.sh
 TEST_PROGS_EXTENDED := in_netns.sh
 TEST_GEN_FILES =  socket
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy
diff --git a/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh b/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh
new file mode 100755
index 000000000000..2d442cdab11e
--- /dev/null
+++ b/tools/testing/selftests/net/test_vxlan_fdb_changelink.sh
@@ -0,0 +1,29 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Check FDB default-remote handling across "ip link set".
+
+check_remotes()
+{
+	local what=$1; shift
+	local N=$(bridge fdb sh dev vx | grep 00:00:00:00:00:00 | wc -l)
+
+	echo -ne "expected two remotes after $what\t"
+	if [[ $N != 2 ]]; then
+		echo "[FAIL]"
+		EXIT_STATUS=1
+	else
+		echo "[ OK ]"
+	fi
+}
+
+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
+check_remotes "fdb append"
+
+ip link set dev vx type vxlan remote 192.0.2.30
+check_remotes "link set"
+
+ip link del dev vx
+exit $EXIT_STATUS
-- 
2.4.11

Re: [PATCH net v3 0/4] vxlan: Various fixes

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Tue, 18 Dec 2018 13:15:57 +0000

> This patch set contains three fixes for the vxlan driver.
> 
> Patch #1 fixes handling of offload mark on replaced VXLAN FDB entries. A
> way to trigger this is to replace the FDB entry with one that can not be
> offloaded. A future patch set should make it possible to veto such FDB
> changes. However the FDB might still fail to be offloaded due to another
> issue, and the offload mark should reflect that.
> 
> Patch #2 fixes problems in __vxlan_dev_create() when a call to
> rtnl_configure_link() fails. These failures would be tricky to hit on a
> real system, the most likely vector is through an error in vxlan_open().
> However, with the abovementioned vetoing patchset, vetoing the created
> entry would trigger the same problems (and be easier to reproduce).
> 
> Patch #3 fixes a problem in vxlan_changelink(). In situations where the
> default remote configured in the FDB table (if any) does not exactly
> match the remote address configured at the VXLAN device, changing the
> remote address breaks the default FDB entry. Patch #4 is then a self
> test for this issue.
 ...

Series applied, thank you.