Re: [PATCH batadv 2/4] batman-adv: tt: only update flags for TT events

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 7110 bytes --]

On Wednesday, 7 October 2026 12:54:50 CEST sashiko-bot@kernel.org wrote:
[...]
> [Severity: High]
> While this masks BATADV_TT_CLIENT_ROAM for the incremental event, doesn't the
> flag remain corrupted in the local TT entry (common->flags)?
> 
> Earlier in batadv_tt_local_refresh(), there is this structure:
> 
>                 if (common->flags & BATADV_TT_CLIENT_PENDING) {
>                         ...
>                         common->flags &= ~BATADV_TT_CLIENT_PENDING;
>                         announce = true;
>                 } else if (common->flags & BATADV_TT_CLIENT_ROAM) {
>                         ...
>                         common->flags &= ~BATADV_TT_CLIENT_ROAM;
>                         *roamed_back = true;
>                 }
> 
> When a client roams away, batadv_tt_local_mark_removed() sets both
> BATADV_TT_CLIENT_ROAM and BATADV_TT_CLIENT_PENDING.
> 
> When the client returns, batadv_tt_local_refresh() clears the PENDING flag
> but skips the else if branch. This leaves BATADV_TT_CLIENT_ROAM set
> locally and *roamed_back as false.
> 
> Consequently, won't any full TT dump (batadv_tt_tvlv_generate) erroneously
> announce the returning client as roaming, causing other nodes to mistakenly
> purge their originators for this client or blackhole traffic?
> 
> Furthermore, does the skipped roamed_back assignment prevent
> batadv_tt_local_add_roam() from freeing the global entry and reclaiming the
> client?

Antonio,

this is exactly what I meant in 
https://lore.kernel.org/batman/20261007-tt-avoid-unhashed-event-v1-0-01c763657916@narfation.org/T/#m35e1aeec45790b26b17d4f4297d77bd8d5b90450 
and which was also previously reported at 
https://www.open-mesh.org/issues/482#change-1886


Maybe needed steps are:


Clear ROAM for pending clients after readd (from local and whenever the global 
still has the client_roam set):


diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index e46040fd745ea8ed0d5b3b497e86b6f37c9c8e25..89098976e31d178829fb575546679178b0d7d0eb 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -865,13 +865,18 @@ static void batadv_tt_local_add_roam(struct batadv_priv *bat_priv,
 		return;
 
 	/* Check whether it is a roaming, but don't do anything if the roaming
-	 * process has already been handled
+	 * process has already been handled.
+	 *
+	 * A client which roamed back must always be reclaimed from the
+	 * originator(s) which sent the ROAMING_ADV. The global entry created by
+	 * this ROAMING_ADV might still carry BATADV_TT_CLIENT_ROAM because the
+	 * new originator did not yet announce the client.
 	 */
-	scoped_guard(spinlock_bh, &tt_global->common.flags_lock) {
-		if (tt_global->common.flags & BATADV_TT_CLIENT_ROAM)
-			return;
+	if (!roamed_back) {
+		scoped_guard(spinlock_bh, &tt_global->common.flags_lock) {
+			if (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.
 			 *
@@ -939,6 +944,7 @@ static bool batadv_tt_local_add_existing(struct batadv_priv *bat_priv,
 					 bool *roamed_back)
 {
 	struct batadv_tt_common_entry *common = &tt_local->common;
+	bool readded = false;
 
 	tt_local->last_seen = jiffies;
 
@@ -953,10 +959,14 @@ static bool batadv_tt_local_add_existing(struct batadv_priv *bat_priv,
 			 * flag can be reset like it was never enqueued
 			 */
 			common->flags &= ~BATADV_TT_CLIENT_PENDING;
-
-			return true;
+			readded = true;
 		}
 
+		/* a pending client can also carry the ROAM flag when it was
+		 * removed because of a roaming_advertisement. It must not keep
+		 * it after being re-added or it would no longer be considered
+		 * as served by this node
+		 */
 		if (common->flags & BATADV_TT_CLIENT_ROAM) {
 			batadv_dbg(BATADV_DBG_TT, bat_priv,
 				   "Roaming client %pM (vid: %d) came back to its original location\n",
@@ -971,7 +981,7 @@ static bool batadv_tt_local_add_existing(struct batadv_priv *bat_priv,
 		}
 	}
 
-	return false;
+	return readded;
 }
 
 /**





Only transmit ROAM events with DEL:


diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 89098976e31d178829fb575546679178b0d7d0eb..b14593b6ec6d52986bbb23647420391520e37dc5 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -696,9 +696,17 @@ static void __batadv_tt_local_event(struct batadv_priv *bat_priv,
 	ether_addr_copy(tt_change_node->change.addr, common->addr);
 	tt_change_node->change.vid = htons(common->vid);
 
-	tt_change_node->change.flags = flags;
 	del_op_requested = flags & BATADV_TT_CLIENT_DEL;
 
+	/* the ROAM flag is only meaningful for the receiver in combination
+	 * with a DEL. An ADD with ROAM would be interpreted as a roaming
+	 * advertisement
+	 */
+	if (!del_op_requested)
+		flags &= ~BATADV_TT_CLIENT_ROAM;
+
+	tt_change_node->change.flags = flags;
+
 	/* check for ADD+DEL, DEL+ADD, ADD+ADD or DEL+DEL events */
 	spin_lock_bh(&bat_priv->tt.changes_list_lock);
 	changes = READ_ONCE(bat_priv->tt.local_changes);
@@ -3405,8 +3413,12 @@ static bool batadv_tt_local_valid(void *entry_ptr,
 	if (tt_flags & BATADV_TT_CLIENT_NEW)
 		return false;
 
+	/* the entry is (still) announced. A ROAM flag of an entry which is
+	 * pending to be removed must not be transmitted. Otherwise, the
+	 * receiver would handle it like a roaming advertisement
+	 */
 	if (flags)
-		*flags = tt_flags;
+		*flags = tt_flags & ~BATADV_TT_CLIENT_ROAM;
 
 	return true;
 }




Ignore ROAM for an ADD received for a global entry update via event:


diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index b14593b6ec6d52986bbb23647420391520e37dc5..8dd5822c9c7128e67cd8abdb4ddce5dc0c6eec9e 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -3996,22 +3996,31 @@ static void _batadv_tt_update_changes(struct batadv_priv *bat_priv,
 				      struct batadv_tvlv_tt_change *tt_change,
 				      u16 tt_num_changes, u8 ttvn)
 {
+	u8 flags;
 	int roams;
 	int i;
 
 	for (i = 0; i < tt_num_changes; i++) {
-		if ((tt_change + i)->flags & BATADV_TT_CLIENT_DEL) {
-			roams = (tt_change + i)->flags & BATADV_TT_CLIENT_ROAM;
+		flags = (tt_change + i)->flags;
+
+		if (flags & BATADV_TT_CLIENT_DEL) {
+			roams = flags & BATADV_TT_CLIENT_ROAM;
 			batadv_tt_global_del(bat_priv, orig_node,
 					     (tt_change + i)->addr,
 					     ntohs((tt_change + i)->vid),
 					     "tt removed by changes",
 					     roams);
 		} else {
+			/* an announced client cannot have roamed away from
+			 * orig_node. Only a ROAMING_ADV is allowed to add a
+			 * global entry with the ROAM flag
+			 */
+			flags &= ~BATADV_TT_CLIENT_ROAM;
+
 			if (!batadv_tt_global_add(bat_priv, orig_node,
 						  (tt_change + i)->addr,
 						  ntohs((tt_change + i)->vid),
-						  (tt_change + i)->flags, ttvn))
+						  flags, ttvn))
 				/* In case of problem while storing a
 				 * global_entry, we stop the updating
 				 * procedure without committing the

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]