[PATCH batadv 2/4] batman-adv: tt: don't directly clear NEW flag when removing local entry

The conversion of the translation table flags to atomic_t turned the pure
BATADV_TT_CLIENT_NEW test in batadv_tt_local_remove() into a
test-and-clear. This is not needed for the atomic flag handling and instead
breaks the local translation table size accounting.

BATADV_TT_CLIENT_NEW is the only marker which tells the commit worker that
an entry still has to be counted. The single increment site is the NEW ->
!NEW transition in batadv_tt_local_transition_new(), while
batadv_tt_local_purge_pending_clients() unconditionally decrements every
still hashed entry which carries BATADV_TT_CLIENT_PENDING. Clearing the
flag outside of batadv_tt_local_transition_new() therefore drops the
increment while keeping the decrement:

  CPU A batadv_tt_local_remove()
    atomic_fetch_andnot(NEW) -> old_flags has NEW
    batadv_tt_local_event(..., BATADV_TT_CLIENT_DEL)
  CPU B batadv_tt_local_remove() (entry found before A unlinked it)
    atomic_fetch_andnot(NEW) -> old_flags has no NEW
    batadv_tt_local_set_pending() -> atomic_or(PENDING)
  CPU C commit worker
    batadv_tt_local_transition_new() -> no NEW, no size_inc
    batadv_tt_local_purge_pending_clients() -> size_dec + hlist_del_rcu
  CPU A batadv_tt_local_remove()
    batadv_hash_remove() -> NULL

Restore the read-only test at the start of the function and add an explicit
batadv_tt_local_size_dec() for the !NEW case. Losing the race against
another remover is already handled by batadv_hash_remove() returning NULL.

Reported-by: Sashiko <[email protected]>
Fixes: 8b4cee3c9355 ("batman-adv: tt: use atomic flag modifications")
Signed-off-by: Sven Eckelmann <[email protected]>
---
 net/batman-adv/translation-table.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 99887237..248b2966 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -1416,13 +1416,19 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
 		atomic_or(BATADV_TT_CLIENT_ROAM, &tt_local_entry->common.flags);
 	}
 
-	old_flags = atomic_fetch_andnot(BATADV_TT_CLIENT_NEW,
-					&tt_local_entry->common.flags);
-	if (!(old_flags & BATADV_TT_CLIENT_NEW)) {
+	/* This must stay a read-only test while the entry is still hashed:
+	 * clearing the flag here would make a concurrent remover of the same
+	 * entry pick the branch below and mark a not yet committed entry as
+	 * pending. A parallel batadv_tt_local_purge_pending_clients()
+	 * then calls batadv_tt_local_size_dec() for this entry even when
+	 * it was never counted by via batadv_tt_local_set_flags()
+	 */
+	if (!(atomic_read(&tt_local_entry->common.flags) & BATADV_TT_CLIENT_NEW)) {
 		batadv_tt_local_set_pending(bat_priv, tt_local_entry, flags,
 					    message);
 		goto out;
 	}
+
 	/* if this client has been added right now, it is possible to
 	 * immediately purge it
 	 */
@@ -1436,6 +1442,15 @@ u16 batadv_tt_local_remove(struct batadv_priv *bat_priv, const u8 *addr,
 	if (!tt_removed_node)
 		goto out;
 
+	/* batadv_tt_local_transition_new() may have committed the entry and
+	 * thus counted it in the local table size since the BATADV_TT_CLIENT_NEW
+	 * check above.
+	 */
+	old_flags = atomic_fetch_andnot(BATADV_TT_CLIENT_NEW,
+					&tt_local_entry->common.flags);
+	if (!(old_flags & BATADV_TT_CLIENT_NEW))
+		batadv_tt_local_size_dec(bat_priv, tt_local_entry->common.vid);
+
 	/* drop reference of remove hash entry */
 	batadv_tt_local_entry_put(tt_local_entry);
 

-- 
2.47.3