Re: [PATCH net-next 5/9] batman-adv: tt: only queue local del event after successful unlink

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete, and the reordering opens a new window.
- [Low] The commit fixes a TT announcement bug but has no Fixes: tag.

--

batman-adv: tt: only queue local del event after successful unlink

This commit moves the BATADV_TT_CLIENT_DEL event in
batadv_tt_local_remove_now() so it runs after batadv_hash_remove(). A
delete event is now only queued when this exact entry was unlinked. The
goal is to stop cancelling the pending add event of another local entry
with the same MAC and VLAN.

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

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=8
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>

[Severity: Low]

This isn't a runtime bug, but should this have a Fixes: tag?

The problem seems to come from bbfb0d2cf035 ("batman-adv: tt: remove only
the entry which was looked up from the hash"). That commit changed the
unlink in batadv_tt_local_remove() from batadv_compare_tt to
batadv_compare_tt_entry. It kept the unconditional delete event in front
of the unlink.

931de98d3960 ("batman-adv: tt: extract immediate purge of a local entry")
only moved that code into batadv_tt_local_remove_now().

Other fixes in the same series have Fixes: tags, for example "batman-adv:
tt: queue local DEL event under bucket lock". Perhaps something like:

Fixes: bbfb0d2cf035 ("batman-adv: tt: remove only the entry which was looked up from the hash")

> diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> index 354d9416c1b75..c904d67791f8f 100644
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c
> @@ -1496,8 +1496,6 @@ batadv_tt_local_remove_now(struct batadv_priv *bat_priv,
>  	struct batadv_tt_common_entry *common = &tt_local_entry->common;
>  	struct hlist_node *tt_removed_node;
>  
> -	batadv_tt_local_event(bat_priv, tt_local_entry, BATADV_TT_CLIENT_DEL);
> -
>  	/* remove exactly this object when still present in hash */
>  	tt_removed_node = batadv_hash_remove(bat_priv->tt.local_hash,
>  					     batadv_compare_tt_entry,
> @@ -1505,6 +1503,8 @@ batadv_tt_local_remove_now(struct batadv_priv *bat_priv,
>  	if (!tt_removed_node)
>  		return;
>  
> +	batadv_tt_local_event(bat_priv, tt_local_entry, BATADV_TT_CLIENT_DEL);
> +

[Severity: High]

Does this reordering open a new window in which a replacement entry's add
event gets dropped?

batadv_hash_remove() takes and releases the bucket list_lock before it
returns. batadv_tt_local_event() only takes tt.changes_list_lock after
that. Nothing seems to serialize this gap against batadv_tt_local_add()
called from batadv_interface_tx() for the same MAC and VID.

The remove side can be reached from a remote TT announcement:

batadv_tt_global_add()
  batadv_tt_global_purge_local()
    batadv_tt_local_remove()
      batadv_tt_local_remove_now()

Consider entry E, which is still NEW and has a queued add event:

CPU0 (remove path)
batadv_hash_remove(E)
  /* E unlinked, its add event still queued */

CPU1 (batadv_tt_local_add())
batadv_tt_local_hash_find()      /* misses E */
batadv_tt_local_create()         /* new entry E' */
batadv_hash_add(E')
batadv_tt_local_event(E', BATADV_NO_FLAGS)
  /* same addr/vid as E's pending add, so only flags are updated */

CPU0
batadv_tt_local_event(E, BATADV_TT_CLIENT_DEL)
  /* matches the remaining add by addr/vid only */
  if (del_op_requested != del_op_entry) {
      list_del(&entry->list);
      kmem_cache_free(batadv_tt_change_cache, entry);
      changes--;

E' is then in tt.local_hash with BATADV_TT_CLIENT_NEW set and no add
event queued. The commit message says this should be prevented.

Later packets from the client do not queue it again, because
batadv_tt_local_add_existing() returns false for a NEW entry that is not
PENDING.

If local_changes drops to 0, batadv_tt_local_commit_changes_nolock()
returns early and E' stays unannounced. A later unrelated commit would
move E' into the CRC through batadv_tt_local_transition_new() without an
add diff. Neighbours would then see a CRC mismatch and fall back to full
table requests.

Before this patch the delete event was queued while E was still in the
hash. An add that ran after the unlink would then get a fresh add event
that nothing cancels.

The later patches in the series don't seem to change this part of
batadv_tt_local_remove_now().

Could the delete event instead be queued inside the same bucket list_lock
critical section as the unlink, and only when this exact object was
found? batadv_tt_local_set_pending() in "batman-adv: tt: queue local DEL
event under bucket lock" already takes list_lock and then
changes_list_lock in that order.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930094558.3723766-2-sw%40simonwunderlich.de