[B.A.T.M.A.N.] [PATCH] batman-adv: don't rely on positions in struct for hashing

[B.A.T.M.A.N.] [PATCH] batman-adv: don't rely on positions in struct for hashing

[Simon Wunderlich]

The hash functions in the bridge loop avoidance code expects the
VLAN vid to be right after the mac address, but this is not guaranteed.

Fix this by explicitly hashing over the right fields of the struct.

Reported-by: Marek Lindner <lindner_marek@yahoo.de>
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
 bridge_loop_avoidance.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
index 0921509..7e62c79 100644
--- a/bridge_loop_avoidance.c
+++ b/bridge_loop_avoidance.c
@@ -37,18 +37,26 @@ static void batadv_bla_periodic_work(struct work_struct *work);
 static void batadv_bla_send_announce(struct batadv_priv *bat_priv,
 				     struct batadv_backbone_gw *backbone_gw);
 
+static inline void hash_bytes(uint32_t *hash, void *data, uint32_t size)
+{
+	const unsigned char *key = data;
+	int i;
+
+	for (i = 0; i < size; i++) {
+		*hash += key[i];
+		*hash += (*hash << 10);
+		*hash ^= (*hash >> 6);
+	}
+}
+
 /* return the index of the claim */
 static inline uint32_t batadv_choose_claim(const void *data, uint32_t size)
 {
-	const unsigned char *key = data;
+	struct batadv_claim *claim = (struct batadv_claim *)data;
 	uint32_t hash = 0;
-	size_t i;
 
-	for (i = 0; i < ETH_ALEN + sizeof(short); i++) {
-		hash += key[i];
-		hash += (hash << 10);
-		hash ^= (hash >> 6);
-	}
+	hash_bytes(&hash, &claim->addr, sizeof(claim->addr));
+	hash_bytes(&hash, &claim->vid, sizeof(claim->vid));
 
 	hash += (hash << 3);
 	hash ^= (hash >> 11);
@@ -61,15 +69,11 @@ static inline uint32_t batadv_choose_claim(const void *data, uint32_t size)
 static inline uint32_t batadv_choose_backbone_gw(const void *data,
 						 uint32_t size)
 {
-	const unsigned char *key = data;
+	struct batadv_claim *claim = (struct batadv_claim *)data;
 	uint32_t hash = 0;
-	size_t i;
 
-	for (i = 0; i < ETH_ALEN + sizeof(short); i++) {
-		hash += key[i];
-		hash += (hash << 10);
-		hash ^= (hash >> 6);
-	}
+	hash_bytes(&hash, &claim->addr, sizeof(claim->addr));
+	hash_bytes(&hash, &claim->vid, sizeof(claim->vid));
 
 	hash += (hash << 3);
 	hash ^= (hash >> 11);
-- 
1.7.10

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: don't rely on positions in struct for hashing

[Antonio Quartulli]

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

On Thu, Aug 30, 2012 at 06:22:27PM +0200, Simon Wunderlich wrote:
> The hash functions in the bridge loop avoidance code expects the
> VLAN vid to be right after the mac address, but this is not guaranteed.
> 
> Fix this by explicitly hashing over the right fields of the struct.
> 

What about creating a new structure like

struct {
	uint8_t mac[ETH_ALEN];
	short vid;
}

to be used as first field in the batadv_claim object? Then you can easily hash
the first 10 bytes in one shot.
This would also help to avoid code duplication in the future (TT will support
VLAN tagging sooner or later and will need the same trick).

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: don't rely on positions in struct for hashing

[Antonio Quartulli]

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

On Thu, Aug 30, 2012 at 06:33:52PM +0200, Antonio Quartulli wrote:
> On Thu, Aug 30, 2012 at 06:22:27PM +0200, Simon Wunderlich wrote:
> > The hash functions in the bridge loop avoidance code expects the
> > VLAN vid to be right after the mac address, but this is not guaranteed.
> > 
> > Fix this by explicitly hashing over the right fields of the struct.
> > 
> 
> What about creating a new structure like
> 
> struct {
> 	uint8_t mac[ETH_ALEN];
> 	short vid;
> }
> 
> to be used as first field in the batadv_claim object? Then you can easily hash
> the first 10 bytes in one shot.
> This would also help to avoid code duplication in the future (TT will support
> VLAN tagging sooner or later and will need the same trick).

Sorry,
but my proposal is wrong. In the new structure we could still have some padding
between the two fields (and we can't know), therefore we cannot hash 10bytes in
one shot as I said.

Drop my idea :)

Cheers,




-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: don't rely on positions in struct for hashing

[Marek Lindner]

On Friday, August 31, 2012 00:22:27 Simon Wunderlich wrote:
> The hash functions in the bridge loop avoidance code expects the
> VLAN vid to be right after the mac address, but this is not guaranteed.
> 
> Fix this by explicitly hashing over the right fields of the struct.
> 
> Reported-by: Marek Lindner <lindner_marek@yahoo.de>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> ---
>  bridge_loop_avoidance.c |   32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)

Applied in revision 7e15c93.

Thanks,
Marek