Message-ID: <20250928171703.GA6416@sol>
Date: Sun, 28 Sep 2025 10:17:03 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Sven Eckelmann <sven@...fation.org>
Cc: Simon Wunderlich <sw@...onwunderlich.de>, kuba@...nel.org,
	davem@...emloft.net, netdev@...r.kernel.org,
	b.a.t.m.a.n@...ts.open-mesh.org
Subject: Re: [PATCH net-next 3/4] batman-adv: keep skb crc32 helper local in
 BLA

On Sun, Sep 28, 2025 at 10:45:12AM +0200, Sven Eckelmann wrote:
> On Saturday, 27 September 2025 22:55:52 CEST Eric Biggers wrote:
> > Hi,
> > 
> > On Tue, Sep 16, 2025 at 02:24:40PM +0200, Simon Wunderlich wrote:
> > > +static __be32 batadv_skb_crc32(struct sk_buff *skb, u8 *payload_ptr)
> > > +{
> > > +	unsigned int to = skb->len;
> > > +	unsigned int consumed = 0;
> > > +	struct skb_seq_state st;
> > > +	unsigned int from;
> > > +	unsigned int len;
> > > +	const u8 *data;
> > > +	u32 crc = 0;
> > > +
> > > +	from = (unsigned int)(payload_ptr - skb->data);
> > > +
> > > +	skb_prepare_seq_read(skb, from, to, &st);
> > > +	while ((len = skb_seq_read(consumed, &data, &st)) != 0) {
> > > +		crc = crc32c(crc, data, len);
> > > +		consumed += len;
> > > +	}
> > > +
> > > +	return htonl(crc);
> > > +}
> > 
> > Has using skb_crc32c() been considered here?
> 
> No. At the time this was written (v3.8), skb_crc32c (v6.16) didnt exist. Also 
> its predecessor __skb_checksum only started its existence in v3.13. And no one 
> noticed it as candidate to replace batadv_skb_crc32 with
> 
> And this patch here was just moving the function between two places - so not 
> introducing new code.
> 
> Do you want to submit a patch to integrate your new function in batman-adv? I 
> only performed a quick-and-dirty test to see if it returns the same results 
> and it seemed to do its job fine.
> 
> diff --git c/net/batman-adv/Kconfig w/net/batman-adv/Kconfig
> index c299e2bc..58c408b7 100644
> --- c/net/batman-adv/Kconfig
> +++ w/net/batman-adv/Kconfig
> @@ -35,6 +35,7 @@ config BATMAN_ADV_BLA
>  	bool "Bridge Loop Avoidance"
>  	depends on BATMAN_ADV && INET
>  	select CRC16
> +	select NET_CRC32C
>  	default y
>  	help
>  	  This option enables BLA (Bridge Loop Avoidance), a mechanism
> diff --git c/net/batman-adv/bridge_loop_avoidance.c w/net/batman-adv/bridge_loop_avoidance.c
> index b992ba12..eef40b6f 100644
> --- c/net/batman-adv/bridge_loop_avoidance.c
> +++ w/net/batman-adv/bridge_loop_avoidance.c
> @@ -12,7 +12,6 @@
>  #include <linux/compiler.h>
>  #include <linux/container_of.h>
>  #include <linux/crc16.h>
> -#include <linux/crc32.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/etherdevice.h>
> @@ -1585,45 +1584,11 @@ int batadv_bla_init(struct batadv_priv *bat_priv)
>  	return 0;
>  }
>  
> -/**
> - * batadv_skb_crc32() - calculate CRC32 of the whole packet and skip bytes in
> - *  the header
> - * @skb: skb pointing to fragmented socket buffers
> - * @payload_ptr: Pointer to position inside the head buffer of the skb
> - *  marking the start of the data to be CRC'ed
> - *
> - * payload_ptr must always point to an address in the skb head buffer and not to
> - * a fragment.
> - *
> - * Return: big endian crc32c of the checksummed data
> - */
> -static __be32 batadv_skb_crc32(struct sk_buff *skb, u8 *payload_ptr)
> -{
> -	unsigned int to = skb->len;
> -	unsigned int consumed = 0;
> -	struct skb_seq_state st;
> -	unsigned int from;
> -	unsigned int len;
> -	const u8 *data;
> -	u32 crc = 0;
> -
> -	from = (unsigned int)(payload_ptr - skb->data);
> -
> -	skb_prepare_seq_read(skb, from, to, &st);
> -	while ((len = skb_seq_read(consumed, &data, &st)) != 0) {
> -		crc = crc32c(crc, data, len);
> -		consumed += len;
> -	}
> -
> -	return htonl(crc);
> -}
> -
>  /**
>   * batadv_bla_check_duplist() - Check if a frame is in the broadcast dup.
>   * @bat_priv: the bat priv with all the mesh interface information
>   * @skb: contains the multicast packet to be checked
> - * @payload_ptr: pointer to position inside the head buffer of the skb
> - *  marking the start of the data to be CRC'ed
> + * @payload_offset: offset in the skb, marking the start of the data to be CRC'ed
>   * @orig: originator mac address, NULL if unknown
>   *
>   * Check if it is on our broadcast list. Another gateway might have sent the
> @@ -1638,16 +1603,18 @@ static __be32 batadv_skb_crc32(struct sk_buff *skb, u8 *payload_ptr)
>   * Return: true if a packet is in the duplicate list, false otherwise.
>   */
>  static bool batadv_bla_check_duplist(struct batadv_priv *bat_priv,
> -				     struct sk_buff *skb, u8 *payload_ptr,
> +				     struct sk_buff *skb, int payload_offset,
>  				     const u8 *orig)
>  {
>  	struct batadv_bcast_duplist_entry *entry;
>  	bool ret = false;
> +	int payload_len;
>  	int i, curr;
>  	__be32 crc;
>  
>  	/* calculate the crc ... */
> -	crc = batadv_skb_crc32(skb, payload_ptr);
> +	payload_len = skb->len - payload_offset;
> +	crc = htonl(skb_crc32c(skb, payload_offset, payload_len, 0));
>  
>  	spin_lock_bh(&bat_priv->bla.bcast_duplist_lock);
>  
> @@ -1727,7 +1694,7 @@ static bool batadv_bla_check_duplist(struct batadv_priv *bat_priv,
>  static bool batadv_bla_check_ucast_duplist(struct batadv_priv *bat_priv,
>  					   struct sk_buff *skb)
>  {
> -	return batadv_bla_check_duplist(bat_priv, skb, (u8 *)skb->data, NULL);
> +	return batadv_bla_check_duplist(bat_priv, skb, 0, NULL);
>  }
>  
>  /**
> @@ -1745,12 +1712,10 @@ bool batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  				    struct sk_buff *skb)
>  {
>  	struct batadv_bcast_packet *bcast_packet;
> -	u8 *payload_ptr;
>  
>  	bcast_packet = (struct batadv_bcast_packet *)skb->data;
> -	payload_ptr = (u8 *)(bcast_packet + 1);
>  
> -	return batadv_bla_check_duplist(bat_priv, skb, payload_ptr,
> +	return batadv_bla_check_duplist(bat_priv, skb, sizeof(*bcast_packet),
>  					bcast_packet->orig);
>  }

It looks like you're already most of the way there, so I suggest you
send the patch.  Thanks!

- Eric

Powered by blists - more mailing lists