Re: [PATCH net-next 12/15] batman-adv: tt: use protected flag modifications

[Sven Eckelmann <sven@narfation.org>]

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

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=12


> > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> > index fbe75e184153..163f90962306 100644
> > --- a/net/batman-adv/translation-table.c
> > +++ b/net/batman-adv/translation-table.c
> 
> [ ... ]
> 
> > @@ -633,8 +652,23 @@ static void batadv_tt_local_add_roam(struct batadv_priv *bat_priv,
> >  	/* Check whether it is a roaming, but don't do anything if the roaming
> >  	 * process has already been handled
> >  	 */
> > -	if (tt_global->common.flags & BATADV_TT_CLIENT_ROAM)
> > -		return;
> > +	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.
> > +			 *
> > +			 * 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;
> > +			tt_global->common.flags |= BATADV_TT_CLIENT_ROAM;
> > +		}
> > +	}
> 
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> Should the changelog mention this ordering fix, and should the patch carry
> a Fixes: tag?

I am not allowed to add Fixes: lines for patches targeting net-next. And this 
is not suitable for net or stable backports. See
https://lore.kernel.org/all/7d73cd74-b040-4a3d-9d78-4ad8fc0a01f4@redhat.com/


> 
> The commit message describes the change as lost-update hardening with a
> self-correcting symptom:
> 
>     This problem can usually be fixed at a later point when the flags are
>     tried to be adjusted again.
> 
>     To reduce the time the wrong flags are used, [...]
> 
> But the new comment above documents something stronger: the store of
> tt_global->roam_at is now moved ahead of publishing
> BATADV_TT_CLIENT_ROAM, and batadv_tt_global_to_purge() is brought under
> the same flags_lock. The same reordering is applied a second time in
> batadv_tt_global_del_roaming().
[...]

The ordering is not too relevant because they are now both under the same 
lock. Which is more important because roam_at is tied to the flag 
BATADV_TT_CLIENT_ROAM. So I dont' seem why they should now be splitted or 
handled separately.


> > @@ -1285,29 +1329,23 @@ int batadv_tt_local_dump(struct sk_buff *msg, struct netlink_callback *cb)
> >  }
> >  
> >  /**
> > - * batadv_tt_local_set_pending() - mark a local TT entry as pending removal
> > + * batadv_tt_local_set_pending_event() - trigger events for TT pending removal
> >   * @bat_priv: the bat priv with all the mesh interface information
> >   * @tt_local_entry: local TT entry to mark
> >   * @flags: TT change flags to announce together with the pending removal
> >   * @message: debug message describing the reason for the change
> >   *
> > - * Schedule the TT change announcement and set BATADV_TT_CLIENT_PENDING on the
> > - * entry. The entry is kept in the local table until the next TTVN increment
> > - * so that a consistency-check response can still be answered.
> > + * Schedule the TT change announcement for the entry. The entry is kept in the
> > + * local table until the next TTVN increment so that a consistency-check
> > + * response can still be answered.
> >   */
> 
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> The kernel-doc of batadv_tt_local_set_pending_event() still says
> "@tt_local_entry: local TT entry to mark" and still claims the entry "is
> kept in the local table until the next TTVN increment", but the body only
> calls batadv_tt_local_event() and batadv_dbg() now - the
> BATADV_TT_CLIENT_PENDING store moved into both callers.

https://patch.msgid.link/20260901-tt-pending-event-kdoc-v1-1-0454b0799d48@narfation.org

> 
> > @@ -1331,12 +1369,14 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
> >  	struct batadv_tt_local_entry *tt_local_entry;
> >  	struct hlist_node *tt_removed_node;
> >  	u16 curr_flags = BATADV_NO_FLAGS;
> > +	bool pending = false;
> >  	u16 flags;
> >  
> >  	tt_local_entry = batadv_tt_local_hash_find(bat_priv, addr, vid);
> >  	if (!tt_local_entry)
> >  		goto out;
> >  
> > +	spin_lock_bh(&tt_local_entry->common.flags_lock);
> >  	curr_flags = tt_local_entry->common.flags;
> >  
> >  	flags = BATADV_TT_CLIENT_DEL;
> > @@ -1351,10 +1391,17 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
> >  	}
> >  
> >  	if (!(tt_local_entry->common.flags & BATADV_TT_CLIENT_NEW)) {
> > -		batadv_tt_local_set_pending(bat_priv, tt_local_entry, flags,
> > -					    message);
> > +		tt_local_entry->common.flags |= BATADV_TT_CLIENT_PENDING;
> > +		pending = true;
> > +	}
> > +	spin_unlock_bh(&tt_local_entry->common.flags_lock);
> > +
> > +	if (pending) {
> > +		batadv_tt_local_set_pending_event(bat_priv, tt_local_entry, flags,
> > +						  message);
> >  		goto out;
> >  	}
> 
> Does this invert the publication order that the old
> batadv_tt_local_set_pending() guaranteed?
> 
> Previously the DEL change event was queued first and
> BATADV_TT_CLIENT_PENDING was set afterwards. Now the flag becomes
> visible before batadv_tt_local_event() runs, and
> batadv_tt_local_remove() holds neither the hash bucket list_lock nor
> bat_priv->tt.commit_lock:
> 
> CPU0                                    CPU1
> batadv_tt_local_remove()
>   spin_lock_bh(&...flags_lock);
>   flags |= BATADV_TT_CLIENT_PENDING;
>   spin_unlock_bh(&...flags_lock);
>                                         batadv_tt_local_commit_changes()
>                                           batadv_tt_local_purge_pending_clients()
>                                             hlist_del_rcu(&tt_common->hash_entry);
>                                           batadv_tt_local_update_crc()
>                                           atomic_inc(&bat_priv->tt.vn);
>   batadv_tt_local_set_pending_event()
>     batadv_tt_local_event()   /* DEL queued only now */
> 
> Can the client then vanish from the local table and from the CRC of TTVN
> N while no DEL change was announced for TTVN N, leaving the neighbours to
> recover via a full table TT request after their CRC check fails?
> 
> The sibling caller batadv_tt_local_purge_list() does not have this
> problem because batadv_tt_local_purge() holds the bucket list_lock around
> it, but batadv_tt_local_remove() is also reachable from received TT TVLV
> and roaming data via batadv_tt_global_add(), so both sides can run
> concurrently.
> 
> Would it help to set BATADV_TT_CLIENT_PENDING after
> batadv_tt_local_event() has queued the change, or to publish the flag and
> the event under a common lock?

The old code didn't guarantee anything and wasn't really ensuring the order 
other callers see it. And it is unfortunately not too trivial as moving the 
position because it is not known earlier if the entry is pending or not. 
Something more like this would then be required:


diff --git i/net/batman-adv/translation-table.c w/net/batman-adv/translation-table.c
index 1fcbe417..b008c8b1 100644
--- i/net/batman-adv/translation-table.c
+++ w/net/batman-adv/translation-table.c
@@ -1498,7 +1498,9 @@ batadv_tt_local_set_pending_event(struct batadv_priv *bat_priv,
 
 /**
  * batadv_tt_local_mark_removed() - mark a local entry as removed
+ * @bat_priv: the bat priv with all the mesh interface information
  * @tt_local_entry: local TT entry to mark
+ * @message: message to append to the log on deletion
  * @roaming: true if the deletion is due to a roaming event
  * @curr_flags: pointer to store the flags of the entry before it was marked
  *
@@ -1506,23 +1508,46 @@ batadv_tt_local_set_pending_event(struct batadv_priv *bat_priv,
  * ttvn increment, false if it can be purged immediately.
  */
 static bool
-batadv_tt_local_mark_removed(struct batadv_tt_local_entry *tt_local_entry,
-			     bool roaming, u16 *curr_flags)
+batadv_tt_local_mark_removed(struct batadv_priv *bat_priv,
+			     struct batadv_tt_local_entry *tt_local_entry,
+			     const char *message, bool roaming, u16 *curr_flags)
 {
+	spinlock_t *list_lock; /* protects write access to the hash lists */
 	struct batadv_tt_common_entry *common = &tt_local_entry->common;
+	struct batadv_hashtable *hash = bat_priv->tt.local_hash;
 	bool pending = false;
+	u16 flags;
+	u32 i;
 
-	scoped_guard(spinlock_bh, &common->flags_lock) {
-		*curr_flags = common->flags;
+	flags = BATADV_TT_CLIENT_DEL;
+	/* if this global entry addition is due to a roaming, the node has to
+	 * mark the local entry as "roamed" in order to correctly reroute
+	 * packets later
+	 */
+	if (roaming)
+		flags |= BATADV_TT_CLIENT_ROAM;
 
-		/* mark the local client as ROAMed */
-		if (roaming)
-			common->flags |= BATADV_TT_CLIENT_ROAM;
+	i = batadv_choose_tt(tt_local_entry, hash->size);
+	list_lock = &hash->list_locks[i];
 
-		if (!(common->flags & BATADV_TT_CLIENT_NEW)) {
-			common->flags |= BATADV_TT_CLIENT_PENDING;
-			pending = true;
+	scoped_guard(spinlock_bh, list_lock) {
+		scoped_guard(spinlock_bh, &common->flags_lock) {
+			*curr_flags = common->flags;
+
+			/* mark the local client as ROAMed */
+			if (roaming)
+				common->flags |= BATADV_TT_CLIENT_ROAM;
+
+			if (!(common->flags & BATADV_TT_CLIENT_NEW)) {
+				common->flags |= BATADV_TT_CLIENT_PENDING;
+				pending = true;
+			}
 		}
+
+		if (pending)
+			batadv_tt_local_set_pending_event(bat_priv,
+							  tt_local_entry,
+							  flags, message);
 	}
 
 	return pending;
@@ -1580,24 +1605,13 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
 {
 	struct batadv_tt_local_entry *tt_local_entry;
 	u16 curr_flags;
-	u16 flags;
 
 	tt_local_entry = batadv_tt_local_hash_find(bat_priv, addr, vid);
 	if (!tt_local_entry)
 		return BATADV_NO_FLAGS;
 
-	flags = BATADV_TT_CLIENT_DEL;
-	/* if this global entry addition is due to a roaming, the node has to
-	 * mark the local entry as "roamed" in order to correctly reroute
-	 * packets later
-	 */
-	if (roaming)
-		flags |= BATADV_TT_CLIENT_ROAM;
-
-	if (batadv_tt_local_mark_removed(tt_local_entry, roaming, &curr_flags))
-		batadv_tt_local_set_pending_event(bat_priv, tt_local_entry,
-						  flags, message);
-	else
+	if (!batadv_tt_local_mark_removed(bat_priv,tt_local_entry,
+					  message, roaming, &curr_flags))
 		/* if this client has been added right now, it is possible to
 		 * immediately purge it
 		 */

[...]
> Discovered by: gpt-5-6-sol · Missed by: opus-5, sashiko-gemini · Confirmed by: opus-5
> Can deferring batadv_tt_global_del_orig_list() until after flags_lock is
> dropped lose an originator entry added in between?
> 
> The old code purged the originator list first and cleared
> BATADV_TT_CLIENT_TEMP / BATADV_TT_CLIENT_ROAM afterwards, so a second
> context still saw the flag and did its own purge. Now the cleared flag is
> published before the list is wiped:
> 
> CPU0 batadv_tt_global_add()             CPU1 batadv_tt_global_add()
>   spin_lock_bh(&common->flags_lock);
>   delete = true;
>   common->flags &= ~BATADV_TT_CLIENT_TEMP;
>   spin_unlock_bh(&common->flags_lock);
>                                         spin_lock_bh(&common->flags_lock);
>                                         /* TEMP already clear, so
>                                          * delete stays false
>                                          */
>                                         spin_unlock_bh(&common->flags_lock);
>                                         batadv_tt_global_orig_entry_add()
>   batadv_tt_global_del_orig_list()
>     /* drops every orig entry,
>      * including CPU1's fresh one
>      */
> 
> batadv_tt_global_del_orig_list() takes tt_global_entry->list_lock and
> removes all entries:
> 
> 	spin_lock_bh(&tt_global_entry->list_lock);
> 	head = &tt_global_entry->orig_list;
> 	hlist_for_each_entry_safe(orig_entry, safe, head, list)
> 		_batadv_tt_global_del_orig_entry(tt_global_entry, orig_entry);
> 
> Both callers return true in that case, so does the route via CPU1's
> originator silently disappear until the next global CRC mismatch triggers
> a fresh TT request? batadv_tt_global_add() is reached from received TT
> TVLV data (serialized only per originator by orig_node->tt_lock) and from
> batadv_tt_add_temporary_global_entry(), so two CPUs can process
> announcements for the same client from different originators at the same
> time.

I really wait until you tell me I couldn't solve world hunger and all 
conflicts on the world with this patch.
https://www.open-mesh.org/issues/474

> 
> One more note, on the sibling patch "batman-adv: clarify cut-off in
> batadv_v_neigh_is_sob kernel-doc" in this series:
> 
> Discovered by: gpt-5-6-sol · Missed by: opus-5, sashiko-gemini · Confirmed by: opus-5
> The kernel-doc of batadv_v_neigh_is_sob() in net/batman-adv/bat_v.c says
> the function returns true if the candidate throughput is "more than 3/4 of
> the @reference throughput", while the body computes:
> 
> 	threshold = ifinfo_ref->bat_v.throughput / 4;
> 	threshold = ifinfo_ref->bat_v.throughput - threshold;
> 
> 	ret = ifinfo_cand->bat_v.throughput > threshold;
> 
> With integer division that is ceil(3 * ref / 4) and a strict comparison,
> so for a reference throughput of 5 the threshold becomes 4 and a
> candidate of 4 is rejected although 4 is more than 3/4 * 5. Should the doc
> say the cut-off is the reference minus reference / 4, exclusive?

Out-of-scope. 

Regards,
	Sven

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