Re: [PATCH 10/16] batman-adv: use CRC32C instead of CRC16 in TT code

[Antonio Quartulli <antonio@meshcoding.com>]

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

On Wed, Oct 09, 2013 at 03:11:08PM +0100, David Laight wrote:
> > CRC32C has to be preferred to CRC16 because of its possible
> > HW native support and because of the reduced collision
> > probability. With this change the Translation Table
> > component now uses CRC32C to compute the local and global
> > table checksum.
> ...
> > -/* Calculates the checksum of the local table of a given orig_node */
> > -static uint16_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> > +/**
> > + * batadv_tt_global_crc - calculates the checksum of the local table belonging
> > + *  to the given orig_node
> > + * @bat_priv: the bat priv with all the soft interface information
> > + */
> > +static uint32_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> >  				     struct batadv_orig_node *orig_node)
> ...
> >  	for (i = 0; i < hash->size; i++) {
> >  		head = &hash->table[i];
> > @@ -1435,27 +1437,24 @@ static uint16_t batadv_tt_global_crc(struct batadv_priv *bat_priv,
> >  							     orig_node))
> >  				continue;
> > 
> ...
> > +			crc ^= crc32c(0, tt_common->addr, ETH_ALEN);
> >  		}
> 
> Are you really generating CRC32 of a pile of ethernet MAC addresses
> and the XORing the CRC together?
> That gives the same answer as XORing together the MAC addresses and
> then doing a CRC of the final value.

I was not sure about this since the CRC32 is not a linear operation. However
this routine is not on the fast path, so we can also live with this order.

> So it gives you almost no protection against corruption at all.

The corruption check is for the entire global table.
The global table is the union of the local tables of all the other nodes (each
node has a local and a global table).

The resulting value (the xor of the CRCs) is then compared to the value
sent by whom originated this piece of global table.

In this way each batman-adv node is sure to have the entries that the node
really generated in its local table.

(sorry for using the word table several times..)

Does this clarify? or have I misunderstood your objection?

> 
> ...
> > -/* Calculates the checksum of the local table */
> > -static uint16_t batadv_tt_local_crc(struct batadv_priv *bat_priv)
> > +/**
> > + * batadv_tt_local_crc - calculates the checksum of the local table
> > + * @bat_priv: the bat priv with all the soft interface information
> > + */
> > +static uint32_t batadv_tt_local_crc(struct batadv_priv *bat_priv)
> >  {
> ...
> 
> This looks like a clone of the previous routine.
> Surely you can avoid the code duplication.

Some parts are the same, true. But this was already like this.
We can surely try to improve it later on with another patch.


Regards,


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]