Re: [PATCH net 07/10] batman-adv: tt: only transmit local ROAM flag together with DEL

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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 fix leaves the requester-side client drop in place even
  though the commit message lists it as a symptom, and the message…

--

batman-adv: tt: only transmit local ROAM flag together with DEL

This patch stops local TT entries from sending BATADV_TT_CLIENT_ROAM
unless it comes with BATADV_TT_CLIENT_DEL. The ROAM bit is now cleared
for non-DEL events in __batadv_tt_local_event() and for full table
responses in batadv_tt_local_valid(). The goal is to stop receivers from
treating a plain ADD as a roaming advertisement.

[ ... ]

[Severity: Medium]

Does this fix the requester-side symptom described in the commit
message? The commit message says:

  When the requester is the node which sent the ROAMING_ADV, it then
  marks its own (new) local client as roamed back to the old originator
  and drops it when it is still NEW.

After this change, batadv_tt_local_valid() still reports an entry that
batadv_tt_local_mark_removed() marked ROAM|PENDING as valid. It only
clears the ROAM bit. So the entry still goes out as a plain ADD in the
full table response built by batadv_send_my_tt_response().

Take node B, which sent the ROAMING_ADV for client C to the old
originator A. B gets A's full table response before A's next TT commit:

batadv_tt_fill_gtable()
  batadv_tt_global_del_orig(bat_priv, orig_node, -1, "Received full table")
    /* removes B's ROAM-marked global entry for C if A was the only orig */
  _batadv_tt_update_changes()
    batadv_tt_global_add(..., flags without ROAM, ...)
      out_remove:
      batadv_tt_global_purge_local(bat_priv, tt_global_entry, flags)
        batadv_tt_local_remove(..., flags & BATADV_TT_CLIENT_ROAM) /* false */
          batadv_tt_local_mark_removed()
              if (common->flags & BATADV_TT_CLIENT_NEW)
                  break;
          batadv_tt_local_remove_now()

The NEW check in batadv_tt_local_mark_removed() does not depend on
roaming. B's newly roamed-in local entry for C is therefore still
removed right away by batadv_tt_local_remove_now(). If the entry was
already committed, a DEL is queued instead. B's global table then
points C at A.

On A, the local entry for C is ROAM|PENDING, so batadv_is_my_client()
returns false there as well. Would traffic for C then be disrupted
until C sends another frame through B and batadv_tt_local_add()
recreates the entry?

The first bullet list in the commit message also has a related
statement:

  * batadv_tt_global_purge_local() removes a local entry of the receiver

It is listed there as an effect of an ADD carrying ROAM. However,
batadv_tt_global_add() reaches out_remove and calls
batadv_tt_global_purge_local() on every successful non-multicast global
add, whether or not ROAM is set. ROAM only selects the roaming argument
passed to batadv_tt_local_remove().

Is more needed to keep B from dropping its local client in this case?
If not, should the commit message say that this patch only fixes the
ROAM marking of the global entry and the resulting CRC mismatch?

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