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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · 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).

- [Medium] The new `!tvlv_len` error path in
  batadv_tt_tvlv_container_update() drops the snapshotted changes…
- [Low] The new allocation-failure path in
  batadv_tt_tvlv_container_update() (net/batman-adv/translation-table.c,…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the window
  it leaves open has worse consequences than the commit message says.

--

batman-adv: tt: don't drop local events queued during TVLV update

This patch makes batadv_tt_tvlv_container_update() splice the changes_list
and read/reset local_changes atomically under changes_list_lock. Events
queued after the snapshot then stay pending for the next TT round. If the
TVLV buffer allocation fails, the snapshotted changes are now freed instead
of kept.

Closes: https://sashiko.dev/#/bug/linux-fc286ddd-5bed-4683-b842-ee7a307eeb19

> 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:

    if (READ_ONCE(bat_priv->tt.local_changes) == 0) {
        if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt))
            batadv_tt_tvlv_container_update(bat_priv);
        return;
    }

    batadv_tt_local_transition_new(bat_priv);
    ...
    batadv_tt_local_update_crc(bat_priv);
    atomic_inc(&bat_priv->tt.vn);

__batadv_tt_local_event() only takes changes_list_lock, not commit_lock.
Consider an ADD for a new client queued through
batadv_interface_tx()->batadv_tt_local_add()->__batadv_tt_local_event()
after that check but before the splice. In the ogm_append_cnt branch, the
splice takes the event, announces it under the unchanged TTVN, and sets
local_changes to 0.

Receivers see ttvn == orig_ttvn in batadv_tt_update_orig() and ignore the
attached changes. batadv_tt_local_crc() also skips NEW entries:

    if (tt_flags & BATADV_TT_CLIENT_NEW)
        continue;

So the CRC still matches and no table request is sent.

Later rounds see local_changes == 0, so batadv_tt_local_transition_new()
never runs for this client. Could the client then stay NEW, excluded from
the CRC and from full-table replies via batadv_tt_local_valid(), until some
unrelated local TT event triggers a commit?

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?

> +
>  	tt_diff_len = batadv_tt_len(tt_diff_entries_num);
>
>  	/* if we have too many changes for one packet don't send any

[ ... ]

> @@ -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). That container is self-consistent, so peers take no
action.

local_changes is now 0, so if no further local event arrives the next
rounds take the idle branch:

    if (READ_ONCE(bat_priv->tt.local_changes) == 0) {
        if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt))
            batadv_tt_tvlv_container_update(bat_priv);
        return;
    }

ttvn N is only published once ogm_append_cnt has counted down to 0, a few
OGM intervals after the failure.

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.

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?

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

Suppose round N-1 left last_changeset = D1, and new events D2 are committed
with tt.vn bumped to N. If batadv_tt_prepare_tvlv_local_data() fails here,
D2 is freed and local_changes is already 0. last_changeset still holds D1.

When ogm_append_cnt reaches 0, the idle branch calls
batadv_tt_tvlv_container_update() again with an empty snapshot.
tt_diff_len is 0, so it takes:

    if (!drop_changes && tt_diff_len == 0)
        goto container_register;

That skips the last_changeset refresh, and the container is registered with
ttvn N and the new CRCs.

A peer at orig_ttvn N-1 then sees ttvn - orig_ttvn == 1 with no changes in
batadv_tt_update_orig(). It sends a diff request, not a full table request.

In batadv_send_my_tt_response():

    if (tt_data->flags & BATADV_TT_FULL_TABLE || my_ttvn != req_ttvn ||
        !bat_priv->tt.last_changeset)
        full_table = true;
    else
        full_table = false;

my_ttvn == req_ttvn and last_changeset is non-NULL, so the reply is the old
D1 diff labelled ttvn N. The peer applies it through
batadv_tt_update_changes() and only fetches the full table after a later
CRC mismatch.

The new comment says "the other nodes will have to request the full table
anyway", but here the first request is a diff request. Should this path
free last_changeset and set it to NULL under last_changeset_lock, or force
the next round to bump the TTVN?

>
>  	tt_data->flags = BATADV_TT_OGM_DIFF;
>
>  	if (!drop_changes && tt_diff_len == 0)
>  		goto container_register;

[ ... ]

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