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
