[PATCH net-next v5 5/8] vxlan: leave FDB state and flags unchanged when an update fails

[Ali Firas <alishmery18@gmail.com>]

vxlan_fdb_update_existing() writes the requested state and flags into
the entry before the steps that can still fail: under NLM_F_REPLACE the
nexthop update and the check that refuses non-unicast entries, under
NLM_F_APPEND the allocation of the new remote, and the switchdev
notification. Each of them returns an error with the new state and
flags left in place, and no RTM_NEWNEIGH is sent for them, so the entry
changes while the request reports failure and nothing tells userspace.
For example, an RTM_NEWNEIGH asking to replace the permanent all-zeros
entry with a dynamic one is refused with -EOPNOTSUPP, yet leaves the
entry dynamic, and the ageing timer then deletes it.

Write the state and flags only after the nexthop update, the non-unicast
check and the allocation of the new remote, so that a request any of
them refuses never changes the entry, not even briefly. The switchdev
notification reports the new state, so it still follows the write; save
both fields on entry and put them back if a switchdev driver vetoes it.
A successful update ends in the same state and sends the same
notification as before. The ageing timer reads the state and flags
without the hash lock (see commit 7308ef8b4a67 ("vxlan: Stop rejecting
dynamic FDB entries with a nexthop ID")), so it can still age out an
entry that a concurrent successful update makes static; that race
predates this patch and is not addressed here.

The patch "vxlan: vnifilter: account the default FDB entry to memcg"
later in this series makes the NLM_F_APPEND allocation fail routinely
at a memory cgroup limit, on the path that changes a vnifilter VNI's
default entry; this is ordered before it so that failure leaves the
entry as it was. Like the rest of the series it targets net-next.

Found in the automated review of v4 of this series; the other error
exits were found by auditing the rest of the function. Each exit was
triggered in QEMU on net-next. The refused replace and the failed
nexthop update need only netlink requests; the failed append was
injected with the kernel's fail-nth fault injection, and the switchdev
veto was triggered with a test-only module that vetoes the notification,
as no switchdev hardware was available. In a production build (4 vCPU,
ageing 1 s), 356,026,806 refused requests in 1200 s, in equal parts
replaces of the permanent all-zeros entry refused with -EOPNOTSUPP,
appends to it failing on an injected -ENOMEM, and nexthop replaces of
another entry refused with -EINVAL, let the ageing timer delete the
all-zeros entry 1,193 times; the test re-added it after each deletion.

Tested in QEMU with the same requests: each exit now leaves the state
and flags as they were, and the loop above deleted the entry 0 times in
341,241,414 requests. test_vxlan_vnifiltering.sh, which gains a case
for the refused replace later in this series, passes with and without
KASAN and lockdep, and fib_nexthops.sh passes.

Fixes: 6681712d67ee ("vxlan: generalize forwarding tables")
Fixes: 906dc186e020 ("vxlan fdb replace an existing entry")
Fixes: 61f46fe8c646 ("vxlan: Allow vetoing of FDB notifications")
Fixes: 1274e1cc4226 ("vxlan: ecmp support for mac fdb entries")
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
v5: new patch (review of v4 6/7); says how the gap was found and what it
did in QEMU (sinfo bot); writes the state and flags after the steps that
can fail instead of writing and then reverting them, so a request those
steps refuse never changes them, not even briefly; a switchdev veto
still writes them and puts them back (local review and KCSAN runs).

 drivers/net/vxlan/vxlan_core.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index d11028b37472..2303d652d396 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -987,6 +987,8 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 				     struct netlink_ext_ack *extack)
 {
 	__u16 fdb_flags = (ndm_flags & ~NTF_USE);
+	__u16 old_flags = f->flags;
+	__u16 old_state = f->state;
 	struct vxlan_rdst *rd = NULL;
 	struct vxlan_rdst oldrd;
 	int notify = 0;
@@ -1005,21 +1007,6 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 		return -EOPNOTSUPP;
 	}
 
-	/* Do not allow an externally learned entry to take over an entry added
-	 * by the user.
-	 */
-	if (!(fdb_flags & NTF_EXT_LEARNED) ||
-	    !(f->flags & NTF_VXLAN_ADDED_BY_USER)) {
-		if (f->state != state) {
-			f->state = state;
-			notify = 1;
-		}
-		if (f->flags != fdb_flags) {
-			f->flags = fdb_flags;
-			notify = 1;
-		}
-	}
-
 	if ((flags & NLM_F_REPLACE)) {
 		/* Only change unicasts */
 		if (!(is_multicast_ether_addr(f->key.eth_addr) ||
@@ -1048,6 +1035,23 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 		notify |= rc;
 	}
 
+	/* Do not allow an externally learned entry to take over an entry added
+	 * by the user. The state and flags are written only after the steps
+	 * above that can fail, so that a request they refuse never changes
+	 * them, not even briefly: the ageing timer reads them without the lock.
+	 */
+	if (!(fdb_flags & NTF_EXT_LEARNED) ||
+	    !(f->flags & NTF_VXLAN_ADDED_BY_USER)) {
+		if (f->state != state) {
+			f->state = state;
+			notify = 1;
+		}
+		if (f->flags != fdb_flags) {
+			f->flags = fdb_flags;
+			notify = 1;
+		}
+	}
+
 	if (ndm_flags & NTF_USE)
 		WRITE_ONCE(f->updated, jiffies);
 
@@ -1066,13 +1070,17 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 
 err_notify:
 	if (nhid)
-		return err;
+		goto err_restore;
 	if ((flags & NLM_F_REPLACE) && rc)
 		*rd = oldrd;
 	else if ((flags & NLM_F_APPEND) && rc) {
 		list_del_rcu(&rd->list);
 		call_rcu(&rd->rcu, vxlan_dst_free);
 	}
+err_restore:
+	/* The notification was vetoed: put back the state and flags. */
+	f->state = old_state;
+	f->flags = old_flags;
 	return err;
 }
 
-- 
2.53.0