Re: [PATCH net 2/9] batman-adv: bla: avoid CRC corruption due to parallel claim add

[Sven Eckelmann <sven@narfation.org>]

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

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

[...]
> > @@ -732,43 +734,56 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv,
> >  			kfree(claim);
> >  			return;
> >  		}
> > +
> > +		spin_lock_bh(&backbone_gw->crc_lock);
> > +		backbone_gw->crc ^= claim_crc;
> > +		spin_unlock_bh(&backbone_gw->crc_lock);
> 
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> This isn't an issue introduced by this patch, but how do these relative XOR
> updates interact with the absolute CRC store that still exists in
> batadv_bla_del_backbone_claims() at this commit?

Please, look literally one patch further.
https://lore.kernel.org/r/20260821094813.201800-4-sw@simonwunderlich.de

[...]
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> This is a pre-existing inconsistency rather than something this patch
> introduces, but now that the compare and the swap of claim->backbone_gw
> both happen under claim->backbone_lock here, should the two remaining
> readers be annotated too?
> 
> net/batman-adv/bridge_loop_avoidance.c:batadv_bla_del_backbone_claims() {
> 	...
> 		hlist_for_each_entry_safe(claim, node_tmp,
> 					  head, hash_entry) {
> 			if (claim->backbone_gw != backbone_gw)
> 				continue;
> 	...
> }
> 
> net/batman-adv/bridge_loop_avoidance.c:batadv_bla_answer_request() {
> 	...
> 			/* only own claims are interesting */
> 			if (claim->backbone_gw != backbone_gw)
> 				continue;
> 	...
> }
> 
> Both do a plain load, not even READ_ONCE(), while types.h documents the
> field as:
> 
> 	/** @backbone_lock: lock protecting backbone_gw pointer */
> 	spinlock_t backbone_lock;
> 
> At both sites the pointer is only compared and never dereferenced, and in
> batadv_bla_del_backbone_claims() a stale comparison still keeps the CRC
> parity correct because the deferred batadv_claim_release() XORs the address
> out of whichever backbone the claim points at. So this looks like a
> KCSAN-visible plain-access race with no functional fallout, but would a
> READ_ONCE() or a comment on those two readers be worth adding so the
> documented locking rule matches the code?

No about this patch and definitely not high priority. Still made a ticket: 
https://www.open-mesh.org/issues/465 for Simon

Regards,
	Sven

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