Re: [PATCH net 3/9] batman-adv: bla: prevent CRC corruptions after claim flush

[Sven Eckelmann <sven@narfation.org>]

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

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260821094813.201800-1-sw%40simonwunderlich.de?part=3

[...]
> Discovered by: gpt-5-6-sol · Missed by: opus-5, sashiko-gemini · Confirmed by: opus-5
> This isn't a bug introduced by this patch, but since the patch touches
> exactly this function: is the ownership test in the loop just above the
> removed hunk sufficiently synchronized?
> 
> net/batman-adv/bridge_loop_avoidance.c:batadv_bla_del_backbone_claims() {
> 	...
> 	spin_lock_bh(list_lock);
> 	hlist_for_each_entry_safe(claim, node_tmp, head, hash_entry) {
> 		if (claim->backbone_gw != backbone_gw)
> 			continue;
> 
> 		hlist_del_rcu(&claim->hash_entry);
> 		batadv_claim_put(claim);
> 	}
> 	spin_unlock_bh(list_lock);
> 	...
> }
> 
> claim->backbone_gw is read here with only the claim hash bucket list_lock
> held, but the writer in batadv_bla_add_claim() only takes
> claim->backbone_lock:
> 
> net/batman-adv/bridge_loop_avoidance.c:batadv_bla_add_claim() {
> 	...
> 	/* replace backbone_gw atomically and adjust reference counters */
> 	spin_lock_bh(&claim->backbone_lock);
> 	if (claim->backbone_gw != backbone_gw) {
> 		changed = true;
> 
> 		old_backbone_gw = claim->backbone_gw;
> 		kref_get(&backbone_gw->refcount);
> 		claim->backbone_gw = backbone_gw;
> 	} else {
> 	...
> }
> 
> The two paths can run at the same time:
> 
> CPU0 (workqueue)
> batadv_bla_periodic_work()
>   batadv_bla_purge_backbone_gw()
>     batadv_bla_del_backbone_claims()   /* reads claim->backbone_gw */
>     hlist_del_rcu(&backbone_gw->hash_entry);
> 
> CPU1 (rx/tx softirq)
> batadv_bla_rx() / batadv_bla_tx()
>   batadv_bla_process_claim()
>     batadv_handle_claim()
>       batadv_bla_add_claim()           /* writes claim->backbone_gw */
> 
> The same interleaving looks reachable purely from received frames, since
> batadv_handle_announce() calls batadv_bla_send_request() on a CRC mismatch,
> which also calls batadv_bla_del_backbone_claims().
> 
> If the purge observes the stale owner after a racing CLAIM frame has
> reassigned the claim to another backbone gateway, does it unlink a claim
> that is now legitimately owned by that other gateway, so the next mesh frame
> from that client MAC makes batadv_bla_rx() claim it locally and two backbone
> gateways forward broadcast/multicast into the bridged LAN until the
> claim/CRC request protocol resynchronizes?
> 
> In the opposite order, if the purge observes the newly written owner and
> skips a claim that was just reassigned to the gateway being purged, the
> claim survives pointing at a backbone gw that is removed from the backbone
> hash immediately afterwards. Since batadv_bla_purge_claims() only expires
> claims owned by our own address, can such an entry linger and make
> batadv_bla_tx() keep dropping multicast frames from that client MAC until a
> unicast mesh frame triggers a re-claim?
> 
> Refcounting and the CRC XOR accounting do stay balanced here, because
> batadv_claim_release() re-reads the owner under claim->backbone_lock, so
> this looks like a claim table state consistency issue rather than memory
> corruption. 

Minor problem, self healing and not about this patch. Not sure if Simon wants 
to fix it.


> The code is unchanged at the end of the series
> (b32d9323b268). Would taking claim->backbone_lock around the ownership
> test, or comparing against a value read under that lock, be appropriate?

No, this is wrong. You would then complain that this patch is having a TOCTOU 
problem. I will not fall for this trap.

Regards,
	Sven

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