[PATCH batadv 4/4] batman-adv: tt: fix roaming state handling of a local client

Turning the BATADV_TT_CLIENT_ROAM handling in batadv_tt_local_add() into an
atomic single claim changed the behaviour of the roaming check in three
ways.

* the cleanup of the global entry was moved out of the block which is
  only entered when the roaming process was not handled yet.
* the flag is now published before tt_global->roam_at is stamped,
  with the whole batadv_send_roam_adv() loop in between.

Revert this handling but avoid sending the initial roaming-away
announcements when another context took responsibility for it. To keep it
more readable, extract it in a new function.

Reported-by: Sashiko <[email protected]>
Fixes: 8b4cee3c9355 ("batman-adv: tt: use atomic flag modifications")
Signed-off-by: Sven Eckelmann <[email protected]>
---
 net/batman-adv/translation-table.c | 94 +++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 36 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 7907f1c6..b2e15dd1 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -667,6 +667,63 @@ static void batadv_tt_global_free(struct batadv_priv *bat_priv,
 	batadv_tt_global_entry_put(tt_global);
 }
 
+/**
+ * batadv_tt_local_add_roam() - handle roamed clients during batadv_tt_local_add()
+ * @bat_priv: the bat priv with all the mesh interface information
+ * @tt_global: the global TT entry
+ * @roamed_back: whether @tt_global roamed back
+ */
+static void batadv_tt_local_add_roam(struct batadv_priv *bat_priv,
+				     struct batadv_tt_global_entry *tt_global,
+				     bool roamed_back)
+{
+	struct batadv_tt_orig_list_entry *orig_entry;
+	struct hlist_head *head;
+	u16 old_flags;
+
+	if (!tt_global)
+		return;
+
+	/* Check whether it is a roaming, but don't do anything if the roaming
+	 * process has already been handled
+	 */
+	if (atomic_read(&tt_global->common.flags) & BATADV_TT_CLIENT_ROAM)
+		return;
+
+	if (!roamed_back) {
+		/* The global entry has to be marked as ROAMING and has to be
+		 * kept for consistency purpose.
+		 *
+		 * batadv_tt_global_to_purge() evaluates roam_at as soon as it
+		 * observes BATADV_TT_CLIENT_ROAM, so the timeout has to be
+		 * stamped before the flag is published. Otherwise the entry can
+		 * be deleted right away as "Roaming timeout".
+		 */
+		tt_global->roam_at = jiffies;
+
+		/* The flag is claimed via an atomic test-and-set to make sure
+		 * that only a single context announces the roaming
+		 */
+		old_flags = atomic_fetch_or(BATADV_TT_CLIENT_ROAM,
+					    &tt_global->common.flags);
+		if (old_flags & BATADV_TT_CLIENT_ROAM)
+			return;
+	}
+
+	/* These node are probably going to update their tt table */
+	head = &tt_global->orig_list;
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(orig_entry, head, list) {
+		batadv_send_roam_adv(bat_priv, tt_global->common.addr,
+				     tt_global->common.vid,
+				     orig_entry->orig_node);
+	}
+	rcu_read_unlock();
+
+	if (roamed_back)
+		batadv_tt_global_free(bat_priv, tt_global, "Roaming canceled");
+}
+
 /**
  * batadv_tt_local_add() - add a new client to the local table or update an
  *  existing client
@@ -685,14 +742,12 @@ bool batadv_tt_local_add(struct net_device *mesh_iface, const u8 *addr,
 {
 	struct batadv_priv *bat_priv = netdev_priv(mesh_iface);
 	struct batadv_tt_global_entry *tt_global = NULL;
-	struct batadv_tt_orig_list_entry *orig_entry;
 	struct batadv_tt_local_entry *tt_local;
 	struct net *net = dev_net(mesh_iface);
 	struct net_device *in_dev = NULL;
 	struct batadv_meshif_vlan *vlan;
 	bool roamed_back = false;
 	bool iif_is_wifi = false;
-	struct hlist_head *head;
 	int packet_size_max;
 	u8 new_remote_flags;
 	bool ret = false;
@@ -701,7 +756,6 @@ bool batadv_tt_local_add(struct net_device *mesh_iface, const u8 *addr,
 	int table_size;
 	u32 match_mark;
 	u8 clear_flags;
-	u16 old_flags;
 	u8 set_flags;
 	u16 flags;
 
@@ -821,39 +875,7 @@ bool batadv_tt_local_add(struct net_device *mesh_iface, const u8 *addr,
 	batadv_tt_local_event(bat_priv, tt_local, BATADV_NO_FLAGS);
 
 check_roaming:
-	/* Check whether it is a roaming, but don't do anything if the roaming
-	 * process has already been handled. The ROAM flag is claimed via an
-	 * atomic test-and-set to make sure that only a single context handles
-	 * the advertisement for a client
-	 */
-	old_flags = BATADV_TT_CLIENT_ROAM;
-	if (tt_global &&
-	    !(atomic_read(&tt_global->common.flags) & BATADV_TT_CLIENT_ROAM))
-		old_flags = atomic_fetch_or(BATADV_TT_CLIENT_ROAM,
-					    &tt_global->common.flags);
-
-	if (tt_global && !(old_flags & BATADV_TT_CLIENT_ROAM)) {
-		/* These node are probably going to update their tt table */
-		head = &tt_global->orig_list;
-		rcu_read_lock();
-		hlist_for_each_entry_rcu(orig_entry, head, list) {
-			batadv_send_roam_adv(bat_priv, tt_global->common.addr,
-					     tt_global->common.vid,
-					     orig_entry->orig_node);
-		}
-		rcu_read_unlock();
-
-		if (!roamed_back) {
-			/* The global entry was marked as ROAMING and
-			 * has to be kept for consistency purpose
-			 */
-			tt_global->roam_at = jiffies;
-		}
-	}
-
-	/* clean up independent of the roaming advertisement handler */
-	if (tt_global && roamed_back)
-		batadv_tt_global_free(bat_priv, tt_global, "Roaming canceled");
+	batadv_tt_local_add_roam(bat_priv, tt_global, roamed_back);
 
 	/* store the current remote flags before altering them. This helps
 	 * understanding is flags are changing or not

-- 
2.47.3