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

[Sven Eckelmann <sven@narfation.org>]

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

On Tuesday, 27 October 2026 09:55:23 CET netdev-bot+sashiko@kernel.org wrote:
[...]
> > 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/

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

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