Re: [PATCH net-next 08/15] batman-adv: tt: remove only the entry which was looked up from the hash

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

[...]

quoted

So when the looked up entry was already unlinked by another context and a
new entry for the same client was added in the meantime, these two
functions unlink that new entry instead.

Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
Should this carry a Fixes: tag?  The two converted call sites had this
defect since the refcount handling around batadv_hash_remove() was
introduced, and the sibling fixes for the very same call sites did use
Fixes: tags:

  ef72706a0543 ("batman-adv: protect tt_local_entry from concurrent delete
  events") is the Fixes: target of 3d65b9accab4 for the local path

  7683fdc1e886 ("batman-adv: protect the local and the global trans-tables
  with rcu") is the Fixes: target of f131a56880d1 for the global path

Both of those commits already noted that batadv_hash_remove() may delete
"an entry ... which is not the same object as the needle" and only repaired
the reference accounting, which is the part this patch completes.  Without
a trailer, stable tooling has no hint that this is a functional fix rather
than part of the surrounding cleanup.

I am not allowed to add Fixes: lines for patches targeting net-next. See
https://lore.kernel.org/all/7d73cd74-b040-4a3d-9d78-4ad8fc0a01f4@redhat.com/ (local)

quoted

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 88aeefa97db6..66456a2d45e2 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c

@@ -95,6 +95,26 @@ static bool batadv_compare_tt(const struct hlist_node *node, const void *data2)
 	return (tt1->vid == tt2->vid) && batadv_compare_eth(data1, data2);
 }
 
+/**
+ * batadv_compare_tt_entry() - check if a hash node is a specific TT entry
+ * @node: the list element pointer of the TT entry stored in the bucket
+ * @data2: pointer to the tt_common_entry which is looked for
+ *
+ * Unlike batadv_compare_tt(), this only matches the very object which is
+ * passed as @data2 and not just any entry for the same TT client. It is meant
+ * for batadv_hash_remove() callers which must not unlink an entry they did not
+ * look up themselves.
+ *
+ * Return: true if @node belongs to @data2, false otherwise
+ */
+static bool batadv_compare_tt_entry(const struct hlist_node *node,
+				    const void *data2)
+{
+	const struct batadv_tt_common_entry *tt = data2;
+
+	return node == &tt->hash_entry;
+}
+

Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
This isn't a bug introduced by this patch, but was the identical pattern in
batadv_bla_del_claim() (net/batman-adv/bridge_loop_avoidance.c) considered
for the same treatment?  It still looks a claim up by key via
batadv_claim_hash_find() and then removes by key:

Out of scope. And it is missing the completely different promises by this 
function. It would make the code a little bit cleaner but it is definitely 
nothing missing it this patch. A ticket for a potential improvement can be 
found in https://www.open-mesh.org/issues/471.

[...]

Discovered by: gpt-5-6-sol · Missed by: opus-5, sashiko-gemini · Confirmed by: opus-5
This is a pre-existing issue, but should the kernel-doc of
batadv_hash_remove() in net/batman-adv/hash.h be corrected while these
callers are being touched?  It says:

 * Return: returns pointer to data on success, so you can remove the used
 * structure yourself, or NULL on error

while the implementation returns the matched hash node:

		data_save = node;
		hlist_del_rcu(node);

This patch drops the hlist_entry() conversions that used to make the
node-ness of the return value obvious at both tt call sites, so the only
remaining hint is the tt_removed_node variable name.

Out-of-scope for this patch. Follow up patch was submitted
https://patch.msgid.link/20260901-kdoc-hash_del-return-type-v1-1-07b9c26e8df3@narfation.org

Discovered by: gpt-5-6-sol · Missed by: opus-5, sashiko-gemini · Confirmed by: opus-5
This isn't a bug introduced by this patch either, but is the caller side of
batadv_tt_global_free() still racy in a way the identity compare cannot
help with?  batadv_tt_global_del() tests the orig_list emptiness after
batadv_tt_global_del_orig_node() has already dropped
tt_global_entry->list_lock:

		batadv_tt_global_del_orig_node(bat_priv, tt_global_entry,
					       orig_node, message);

		if (hlist_empty(&tt_global_entry->orig_list))
			batadv_tt_global_free(bat_priv, tt_global_entry,
					      message);

Meanwhile batadv_tt_global_orig_entry_add() can add an originator for the
very same object under that lock:

	spin_lock_bh(&tt_global->list_lock);
	...
	hlist_add_head_rcu(&orig_entry->list, &tt_global->orig_list);
	atomic_inc(&tt_global->orig_list_count);

Since the emptiness decision and the hash removal are not atomic, can a
global entry that has just regained a valid originator still be unlinked?
The new comparator matches here because it is the same object.  The
hlist_empty() read also happens outside the lock that the orig_list is
documented to be protected by.

Outside the scope of this patch. For interested contributors
https://www.open-mesh.org/issues/473


[...]

Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
With the identity compare, batadv_hash_remove() can now fail while a
different, live local entry for the same MAC and VLAN is still linked in
bat_priv->tt.local_hash.  The BATADV_TT_CLIENT_DEL change event is queued
before the removal and is not undone on that new failure path.

batadv_tt_local_event() coalesces purely by MAC and VLAN:

[...]

Can the stale DEL then cancel the pending ADD of the surviving entry?

CPU A: batadv_tt_local_remove() (or the purge work) unlinks entry E
CPU B: batadv_tt_local_add() inserts E' for the same addr/vid; the
       batadv_hash_add() happens before its ADD event is queued at
       the add_event label
CPU A: having looked E up earlier via batadv_tt_local_hash_find(), with no
       lock spanning lookup, event and removal, queues DEL and then finds
       no node equal to &E->common.hash_entry

Either ordering ends with both the ADD and the DEL removed from
bat_priv->tt.changes_list, so E' stays in the local hash with
BATADV_TT_CLIENT_NEW set and no queued change.

[...]

Would it make sense to queue the event only after a successful unlink, or to
re-queue the ADD when the removal fails?  The ordering is still the same at
the end of the series, in "batman-adv: tt: decrement count for committed
client on local_remove".

This is not the only way something like this could happen. But ok, submitted a 
patch

https://patch.msgid.link/20260901-tt-del-event-queue-late-v1-1-fc28ca54c342@narfation.org

Regards,
	Sven