Re: [PATCH net 6/9] batman-adv: fix TX priority extraction for BATADV_FORW_MCAST

[Sven Eckelmann <sven@narfation.org>]

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

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

[...]
> > @@ -338,6 +340,9 @@ static netdev_tx_t batadv_interface_tx(struct sk_buff *skb,
> >  
> >  send:
> >  		if (do_bcast && !is_broadcast_ether_addr(ethhdr->h_dest)) {
> > +			/* WARNING batadv_mcast_forw_mode might add more headers
> > +			 * in front of the skb. and might even reallocate the skb
> > +			 */
> >  			forw_mode = batadv_mcast_forw_mode(bat_priv, skb, vid,
> >  							   &mcast_is_routable);
> 
> Discovered by: opus-5 · Missed by: gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
> This isn't a bug introduced by this patch, but would it make sense to put
> this warning into the kernel-doc of the callee instead of (or in addition
> to) the call site?
> 
> Today only batadv_mcast_forw_push() carries the contract in
> net/batman-adv/multicast_forw.c:
> 
>   * Warning: This function may reallocate the skb data buffer via
>   * batadv_mcast_forw_expand_head()/... Any pointer into the skb data (e.g.
>   * obtained from skb->data or eth_hdr()) before this call must be
>   * considered invalid afterwards and has to be reacquired.
> 
> The two callers 

What are the two callers? I only find batadv_mcast_forw_mode_by_count()

> in net/batman-adv/multicast.c do not mention it.
> batadv_mcast_forw_mode_by_count() only says:
> 
>   * For a multicast packet with multiple destination originators, checks which
>   * mode to use. For BATADV_FORW_MCAST it also encapsulates the packet with a
>   * complete batman-adv multicast header.
> 
> and the exported entry point batadv_mcast_forw_mode() (declared in
> net/batman-adv/multicast.h) documents only:
> 
>   * Return: The forwarding mode as enum batadv_forw_mode.
> 
> Since batadv_mcast_forw_mode() is the exported symbol, wouldn't a future
> caller reading its kernel-doc have no way to know that skb->data was pushed
> or that the buffer may have been reallocated?

Will not modify this patch for this request. Linus (or anyone else) can deal 
with it: https://www.open-mesh.org/issues/466

> 
> Related to that: batadv_interface_tx() does not re-acquire ethhdr after
> batadv_mcast_forw_mode() returns, even though the new comment warns about
> reallocation. The only later use, is_broadcast_ether_addr(ethhdr->h_dest),
> is evaluated before the call, 

The later use is before? This is crazy

> so nothing is dereferenced stale right now.
> Is it worth re-acquiring it anyway, or noting in the comment that ethhdr
> must not be used past this point?

Will not modify the patch for this.

> 
> >  			switch (forw_mode) {
> 
> [ ... ]
> 
> > @@ -355,8 +360,6 @@ static netdev_tx_t batadv_interface_tx(struct sk_buff *skb,
> >  		}
> >  	}
> >  
> > -	batadv_skb_set_priority(skb, 0);
> > -
> >  	/* ethernet packet should be broadcasted */
> >  	if (do_bcast) {
> >  		primary_if = batadv_primary_if_get_selected(bat_priv);
> 
> Discovered by: opus-5 · Missed by: gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
> This is a pre-existing issue and not caused by this patch, but while the
> priority handling is being looked at: should the multicast forwarding path
> also set skb->priority?

This is a feature (or maybe minor bugfix) request. Will definitely not deal 
with it here. Documented it at https://www.open-mesh.org/issues/467

Regards,
	Sven

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