Re: [PATCH net] batman-adv: Close OGM aggregation before transmission

[Sven Eckelmann]

Thanks for the fix.

Please don't submit batman-adv changes to netdev directly. And please send the
patch also to the person which introduced the problem. You can use tools like
`b4 prep --auto-to-cc` or just the underlying ./scripts/get_maintainer.pl 

    $ b4 mbox 20260926172600.2394088-1-nicoyip.dev@xxxxxxxxx
    $ ./scripts/get_maintainer.pl 20260926172600.2394088-1-nicoyip.dev@xxxxxxxxxxxxx
    Marek Lindner <marek.lindner@xxxxxxxxxxx> (maintainer:BATMAN ADVANCED)
    Simon Wunderlich <sw@xxxxxxxxxxxxxxxxxx> (maintainer:BATMAN ADVANCED,blamed_fixes:1/1=100%)
    Antonio Quartulli <antonio@xxxxxxxxxxxxx> (maintainer:BATMAN ADVANCED)
    Sven Eckelmann <sven@xxxxxxxxxxxxx> (maintainer:BATMAN ADVANCED,blamed_fixes:1/1=100%)
    "Linus Lüssing" <linus.luessing@xxxxxxxxx> (blamed_fixes:1/1=100%)
    b.a.t.m.a.n@xxxxxxxxxxxxxxxxxxx (moderated list:BATMAN ADVANCED)
    linux-kernel@xxxxxxxxxxxxxxx (open list)

This change should not go directly to net.git. The target tree must be
batadv.git

    $ ./scripts/get_maintainer.pl --scm 20260926172600.2394088-1-nicoyip.dev@xxxxxxxxxxxxx
    [...]
    git https://git.open-mesh.org/batadv.git
    git git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

The last tree is irrelevant for you because it is really unlikely that Linus
Torvalds will apply your patch directly.

> The OGM send worker leaves its forwarding packet on forw_bat_list until
> after batadv_iv_ogm_emit() returns. Aggregation holds forw_bat_list_lock,
> but emission reads and clones the packet without that lock.
> 
> CPU 0 can therefore start emitting a queued packet while CPU 1 takes the
> list lock, finds the same packet and appends another OGM. The sender can
> observe the new packet length before the corresponding direct-link flag
> is set, or clone the skb while its length and payload are being updated.
> This can transmit an OGM with incorrect flags or inconsistent data.
> 
> KCSAN reported:
> 
>   BUG: KCSAN: data-race in batadv_iv_ogm_queue_add / batadv_iv_send_outstanding_bat_ogm_packet
>   write to 0xffff888100fedcf8 of 2 bytes by interrupt on cpu 1:
>   read to 0xffff888100fedcf8 of 2 bytes by task 70 on cpu 2:
>   value changed: 0x00c0 -> 0x00d8

Is there a reproducer and where can I find it?

> Set num_packets to BATADV_MAX_AGGREGATION_PACKETS under the list lock
> before emission. This waits for any ongoing append and makes the existing
> aggregation limit check reject further appends before inspecting mutable
> OGM flags. Emission walks packet_len rather than num_packets, so the
> queued contents are still sent normally. Keep the packet on the list so
> interface purging can still wait for the worker and retain its existing
> ownership of the packet when freeing it.

I might be wrong but this reads a little bit like it was written by an LLM. If
this is the case, please think about following the annotation style described
in https://docs.kernel.org/process/coding-assistants.html

> Backports before Linux 6.15 need count-handling adaptation.

This is wrong. You are depending on 434becf57bdc ("batman-adv: Limit number of
aggregated packets directly") and not the counting adaption. Of course, for
4.10.x-6.14.x, you also need to change the BATADV_MAX_AGGREGATION_PACKETS to
BITS_PER_TYPE(forw_packet->direct_link_flags)

And it doesn't belong in the commit message (like this). Please annotate the
dependencies as described in
https://docs.kernel.org/process/stable-kernel-rules.html#stable-kernel-rules

> Fixes: 9b4aec647a92 ("batman-adv: fix rare race conditions on interface removal")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
>
> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index 53fbdbbe8f4f7..59847299a1237 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c
> @@ -1909,6 +1909,10 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work)
>  		goto out;
>  	}
>  
> +	spin_lock_bh(&bat_priv->forw_bat_list_lock);
> +	forw_packet->num_packets = BATADV_MAX_AGGREGATION_PACKETS;
> +	spin_unlock_bh(&bat_priv->forw_bat_list_lock);
> +

Can you add a small comment above like "mark aggregate as full before forcing
emit" (or something similar).

-- 
Sven Eckelmann <sven@xxxxxxxxxxxxx>