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

[Sven Eckelmann <sven@narfation.org>]

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

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

[...]
> > 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/

> 
> > 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

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