Re: [PATCH net-next 05/10] batman-adv: annotate functions which may reallocate the skbuff

[Sven Eckelmann <sven@narfation.org>]

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

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

> --- Patch [5]: [PATCH net-next 05/10] batman-adv: annotate functions which may reallocate the skbuff ---
[...]
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> This isn't a bug, but the ellipsis in the batadv_dat_snoop_outgoing_arp_request()
> annotation has an extra dot ("batadv_dat_get_vid()/....") compared to the
> "/..." spelling used in the other hunks of this patch.

Feel free to use the extra dot for whatever you want. Will not resend the PR 
or create a patch because of this.

> 
> > + * from skb->data or eth_hdr()) before this call must be considered
> > + * invalid afterwards and has to be reacquired.
> > + *
> >   * Return: true if the message has been sent to the dht candidates, false
> >   * otherwise. In case of a positive return value the message has to be enqueued
> >   * to permit the fallback.
> > @@ -1271,6 +1286,11 @@ bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv,
> >   * @skb: packet to check
> >   * @hdr_size: size of the encapsulation header
> >   *
> > + * Warning: This function may reallocate the skb data buffer via
> > + * batadv_dat_get_vid()/... 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.
> > + *
> >   * Return: true if the request has been answered, false otherwise.
> >   */
> >  bool batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv,
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is the advice to reacquire the pointers correct for the true return path of
> batadv_dat_snoop_incoming_arp_request()?  On that path the skb is not just
> reallocated, it is released:
> 
> net/batman-adv/distributed-arp-table.c:batadv_dat_snoop_incoming_arp_request() {
> 	...
> out:
> 	batadv_dat_entry_put(dat_entry);
> 	if (ret)
> 		kfree_skb(skb);
> 	return ret;
> }

This is outside the scope of this patch. The WARNING is of course only valid 
when there is still an skb to use.

> [ ... ]
> 
> > @@ -1788,6 +1818,11 @@ void batadv_dat_snoop_outgoing_dhcp_ack(struct batadv_priv *bat_priv,
> >   * This function first checks whether the given skb is a valid DHCPACK. If
> >   * so then its source MAC and IP as well as its DHCP Client Hardware Address
> >   * field and DHCP Your IP Address field are added to the local DAT cache.
> > + *
> > + * Warning: This function may reallocate the skb data buffer via
> > + * pskb_may_pull()/batadv_dat_get_vid()/... Any pointer into the skb data
> > + * (e.g.obtained from skb->data or eth_hdr()) before this call must be
>        ^^^^^^^^^^
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Missing space after "e.g." in the batadv_dat_snoop_incoming_dhcp_ack()
> annotation.

Will not resend the PR because of this.

> [ ... ]
> 
> > diff --git a/net/batman-adv/multicast_forw.c b/net/batman-adv/multicast_forw.c
> > index 1404a3b7adfb1..60ec12805742c 100644
> > --- a/net/batman-adv/multicast_forw.c
> > +++ b/net/batman-adv/multicast_forw.c
> > @@ -1080,6 +1080,11 @@ unsigned int batadv_mcast_forw_packet_hdrlen(unsigned int num_dests)
> >   * Tries to expand an skb's headroom so that its head to tail is 1298
> >   * bytes (minimum IPv6 MTU + vlan ethernet header size) large.
> >   *
> > + * Warning: This function may reallocate the skb data buffer via
> > + * skb_cow()/skb_linearize()/... 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.
> > + *
> 
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Are skb_cow() and skb_linearize() the right helpers to name here?
> batadv_mcast_forw_expand_head() calls neither of them; the only
> reallocating primitive in its body is pskb_expand_head():
> 
> net/batman-adv/multicast_forw.c:batadv_mcast_forw_expand_head() {
> 	...
> 	if (skb_headroom(skb) < hdr_size &&
> 	    pskb_expand_head(skb, hdr_size, 0, GFP_ATOMIC) < 0)
> 		return -ENOMEM;
> 
> 	return 0;
> }

The relevant changes were lost when the fixes were removed from the PR. Will 
not resend the PR without this line. The statement will be true when the fixes 
are added.

Regards,
	Sven

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