Re: [PATCH net 09/10] batman-adv: tt: don't drop local events queued during TVLV update

On Tuesday, 27 October 2026 09:55:23 CET netdev-bot+sashiko@kernel.org wrote:
[...]

quoted

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 24054077cf3db..6774484f1c573 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c

@@ -1327,11 +1327,18 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)
 	int tt_diff_entries_num = 0;
 	bool drop_changes = false;
 	size_t tt_extra_len = 0;
+	LIST_HEAD(changes_list);
 	int tt_change_len = 0;
 	int tt_diff_len;
 	u16 tvlv_len;

+	/* atomically get the list of changes */
+	spin_lock_bh(&bat_priv->tt.changes_list_lock);
+	list_splice_init(&bat_priv->tt.changes_list, &changes_list);
 	tt_diff_entries_num = READ_ONCE(bat_priv->tt.local_changes);
+	WRITE_ONCE(bat_priv->tt.local_changes, 0);
+	spin_unlock_bh(&bat_priv->tt.changes_list_lock);

[Severity: Medium]
This isn't a bug introduced by this patch, but the window left open at the
end of the commit message seems to have a worse effect than described. An
event in that window can be lost, not just announced under the wrong TTVN
or CRC.

batadv_tt_local_commit_changes_nolock() makes its decisions from a lockless
read, before this splice happens:

[...]

Would either of these close the window: taking the snapshot in
batadv_tt_local_commit_changes_nolock() before the local_changes check and
passing the list down, or not consuming the list in the non-committing
branch?

Out-of-scope for this patch and handled in another pending patch

https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20261025-tt-tvlv-changes-check-v1-1-c12b246c0956@narfation.org/

[ ... ]

quoted

@@ -1349,19 +1356,23 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)

 	tvlv_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data,
 						     &tt_change, &tt_diff_len);
-	if (!tvlv_len)
+	if (!tvlv_len) {
+		/* the TTVN of the changes is lost and the other nodes will
+		 * have to request the full table anyway
+		 */
+		list_for_each_entry_safe(entry, safe, &changes_list, list) {
+			list_del(&entry->list);
+			kmem_cache_free(batadv_tt_change_cache, entry);
+		}
 		return;
+	}

[Severity: Low]
Does this statement in the commit message hold when the node is idle?

  "The changes can no longer be kept for the next round when the TVLV
  buffer allocation fails. But they would be sent with a skipped TTVN
  anyway, so the other nodes have to request the full table in this case
  either way."

When batadv_tt_prepare_tvlv_local_data() fails here, the caller
batadv_tt_local_commit_changes_nolock() has already bumped tt.vn to N,
updated the CRCs and reset ogm_append_cnt to BATADV_TT_OGM_APPEND_MAX. No
new container is registered, so OGMs keep carrying the old container (ttvn
N-1 with the old CRCs).

[...]

Before this patch, the !tvlv_len return left the list and counter in place.
The next interval then bumped the TTVN to N+1, and peers saw the skipped
TTVN immediately.

Why would the first round fail and the second round not? Please don't start 
with kmalloc-failures - this would open a completely different can of worms 
related to containers and OGMs.

With this change, can peers learn about the committed local changes several
OGM intervals later, with no TTVN skipped at all? Re-queueing the changes
isn't simple because __batadv_tt_local_event() expects at most one queued
change per (addr, vid). Would it be enough to force a TTVN bump or commit in
the next round instead?

What is proposed here?

        if (!tvlv_len) {
                /* the changes are lost and the other nodes have to request the
                 * full table. Announce the new TTVN in the next OGM interval
                 * and don't answer TT requests for it with the stale changeset
                 * of the previous TTVN
                 */
                list_for_each_entry_safe(entry, safe, &changes_list, list) {
                        list_del(&entry->list);
                        kmem_cache_free(batadv_tt_change_cache, entry);
                }

                spin_lock_bh(&bat_priv->tt.last_changeset_lock);
                kfree(bat_priv->tt.last_changeset);
                bat_priv->tt.last_changeset = NULL;
                bat_priv->tt.last_changeset_len = 0;
                spin_unlock_bh(&bat_priv->tt.last_changeset_lock);

                /* announce the already committed TTVN in the next OGM interval
                 * instead of waiting for BATADV_TT_OGM_APPEND_MAX idle rounds
                 */
                atomic_set(&bat_priv->tt.ogm_append_cnt, 0);
                return;
        }

https://www.open-mesh.org/issues/485
https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20261027-tt-tvlv-alloc-fail-v1-1-02058bcc4595@narfation.org/

[Severity: Medium]
Can this error path leave a stale bat_priv->tt.last_changeset that is later
served under the new TTVN?

Which it did even before the patch

[...]

The new comment says "the other nodes will have to request the full table
anyway", but here the first request is a diff request.

Which makes the statements untrue because of "first request"? Which is not
even mentioned in the comment.

Should this path
free last_changeset and set it to NULL under last_changeset_lock, or force
the next round to bump the TTVN?

https://www.open-mesh.org/issues/485

Regards,
	Sven