[v2,1/2] batman-adv: Fix broadcast packet CRC calculation

  So far the crc16 checksum for a batman-adv broadcast data packet, received
on a batman-adv hard interface, was calculated over zero bytes of its
content leading to many incoming broadcast data packets wrongly being
dropped.

This patch fixes this issue by calculating the crc16 over the actual,
complete broadcast payload.

The issue is a regression introduced by
"batman-adv: add broadcast duplicate check".

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
It led to about 60-80% broadcast packet loss to a direct neighbor
with a ping6 with a 1s interval in our scenario, but about no
packet loss with an interval smaller than 0.2s.

Also see: https://projects.universe-factory.net/issues/65 (German)

v2:
~ fixed typo (regrission vs. regression)
~ commit name instead of commit hash in the commit message

 bridge_loop_avoidance.c |    8 ++++----
 routing.c               |    8 +++++++-
 2 files changed, 11 insertions(+), 5 deletions(-)
  

Looks good, although I don't think this will happen very often - it
is only called when receiving packets, and AFAIK there can only be
multiple threads when using multiple interfaces.

Anyway, doing the spinlock shouldn't hurt and might save us some pain
when this function is used more often, or with multiple interfaces, or
cases I don't see now. :)

Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>


On Wed, Oct 17, 2012 at 02:53:05PM +0200, Linus Lüssing wrote:
> Threads in the bottom half of batadv_bla_check_bcast_duplist() might
> otherwise for instance overwrite variables which other threads might
> be using/reading at the same time in the top half, potentially
> leading to messing up the bcast_duplist, possibly resulting in false
> bridge loop avoidance duplicate check decisions.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
> Note: I didn't observe such issues yet, probably because of the 
> usually quite low broadcast data throughput - and on high broadcast
> throughput, then packet loss is usually caused by the limited capacity
> for broadcast packets on 802.11 media anyway.
> 
>  bridge_loop_avoidance.c |   16 +++++++++++-----
>  main.c                  |    1 +
>  types.h                 |    2 ++
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
> index b828875..c6c1c59 100644
> --- a/bridge_loop_avoidance.c
> +++ b/bridge_loop_avoidance.c
> @@ -1263,7 +1263,7 @@ int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  				   struct batadv_bcast_packet *bcast_packet,
>  				   int bcast_packet_len)
>  {
> -	int i, length, curr;
> +	int i, length, curr, ret = 0;
>  	uint8_t *content;
>  	uint16_t crc;
>  	struct batadv_bcast_duplist_entry *entry;
> @@ -1275,6 +1275,8 @@ int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  	/* calculate the crc ... */
>  	crc = crc16(0, content, length);
>  
> +	spin_lock_bh(&bat_priv->bla.bcast_duplist_lock);
> +
>  	for (i = 0; i < BATADV_DUPLIST_SIZE; i++) {
>  		curr = (bat_priv->bla.bcast_duplist_curr + i);
>  		curr %= BATADV_DUPLIST_SIZE;
> @@ -1296,9 +1298,11 @@ int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  		/* this entry seems to match: same crc, not too old,
>  		 * and from another gw. therefore return 1 to forbid it.
>  		 */
> -		return 1;
> +		ret = 1;
> +		goto out;
>  	}
> -	/* not found, add a new entry (overwrite the oldest entry) */
> +	/* not found, add a new entry (overwrite the oldest entry)
> +	 * and allow it, its the first occurence. */
>  	curr = (bat_priv->bla.bcast_duplist_curr + BATADV_DUPLIST_SIZE - 1);
>  	curr %= BATADV_DUPLIST_SIZE;
>  	entry = &bat_priv->bla.bcast_duplist[curr];
> @@ -1307,8 +1311,10 @@ int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  	memcpy(entry->orig, bcast_packet->orig, ETH_ALEN);
>  	bat_priv->bla.bcast_duplist_curr = curr;
>  
> -	/* allow it, its the first occurence. */
> -	return 0;
> +out:
> +	spin_unlock_bh(&bat_priv->bla.bcast_duplist_lock);
> +
> +	return ret;
>  }
>  
>  
> diff --git a/main.c b/main.c
> index 70797de..f54efe9 100644
> --- a/main.c
> +++ b/main.c
> @@ -102,6 +102,7 @@ int batadv_mesh_init(struct net_device *soft_iface)
>  	spin_lock_init(&bat_priv->gw.list_lock);
>  	spin_lock_init(&bat_priv->vis.hash_lock);
>  	spin_lock_init(&bat_priv->vis.list_lock);
> +	spin_lock_init(&bat_priv->bla.bcast_duplist_lock);
>  
>  	INIT_HLIST_HEAD(&bat_priv->forw_bat_list);
>  	INIT_HLIST_HEAD(&bat_priv->forw_bcast_list);
> diff --git a/types.h b/types.h
> index 8a5d84c..7b3d0d7 100644
> --- a/types.h
> +++ b/types.h
> @@ -228,6 +228,8 @@ struct batadv_priv_bla {
>  	struct batadv_hashtable *backbone_hash;
>  	struct batadv_bcast_duplist_entry bcast_duplist[BATADV_DUPLIST_SIZE];
>  	int bcast_duplist_curr;
> +	/* protects bcast_duplist and bcast_duplist_curr */
> +	spinlock_t bcast_duplist_lock;
>  	struct batadv_bla_claim_dst claim_dest;
>  	struct delayed_work work;
>  };
> -- 
> 1.7.10.4
> 
>
  

Good catch! When can I buy you the beer? :D

Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

On Wed, Oct 17, 2012 at 02:53:04PM +0200, Linus Lüssing wrote:
> So far the crc16 checksum for a batman-adv broadcast data packet, received
> on a batman-adv hard interface, was calculated over zero bytes of its
> content leading to many incoming broadcast data packets wrongly being
> dropped.
> 
> This patch fixes this issue by calculating the crc16 over the actual,
> complete broadcast payload.
> 
> The issue is a regression introduced by
> "batman-adv: add broadcast duplicate check".
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
> It led to about 60-80% broadcast packet loss to a direct neighbor
> with a ping6 with a 1s interval in our scenario, but about no
> packet loss with an interval smaller than 0.2s.
> 
> Also see: https://projects.universe-factory.net/issues/65 (German)
> 
> v2:
> ~ fixed typo (regrission vs. regression)
> ~ commit name instead of commit hash in the commit message
> 
>  bridge_loop_avoidance.c |    8 ++++----
>  routing.c               |    8 +++++++-
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
> index a617f2c..b828875 100644
> --- a/bridge_loop_avoidance.c
> +++ b/bridge_loop_avoidance.c
> @@ -1247,8 +1247,8 @@ int batadv_bla_init(struct batadv_priv *bat_priv)
>  /**
>   * batadv_bla_check_bcast_duplist
>   * @bat_priv: the bat priv with all the soft interface information
> - * @bcast_packet: originator mac address
> - * @hdr_size: maximum length of the frame
> + * @bcast_packet: encapsulated broadcast frame plus batman header
> + * @bcast_packet_len: length of encapsulated broadcast frame plus batman header
>   *
>   * check if it is on our broadcast list. Another gateway might
>   * have sent the same packet because it is connected to the same backbone,
> @@ -1261,14 +1261,14 @@ int batadv_bla_init(struct batadv_priv *bat_priv)
>   */
>  int batadv_bla_check_bcast_duplist(struct batadv_priv *bat_priv,
>  				   struct batadv_bcast_packet *bcast_packet,
> -				   int hdr_size)
> +				   int bcast_packet_len)
>  {
>  	int i, length, curr;
>  	uint8_t *content;
>  	uint16_t crc;
>  	struct batadv_bcast_duplist_entry *entry;
>  
> -	length = hdr_size - sizeof(*bcast_packet);
> +	length = bcast_packet_len - sizeof(*bcast_packet);
>  	content = (uint8_t *)bcast_packet;
>  	content += sizeof(*bcast_packet);
>  
> diff --git a/routing.c b/routing.c
> index 6b2104d..5da62df 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -1181,8 +1181,14 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
>  
>  	spin_unlock_bh(&orig_node->bcast_seqno_lock);
>  
> +	/* keep skb linear for crc calculation */
> +	if (skb_linearize(skb) < 0)
> +		goto out;
> +
> +	bcast_packet = (struct batadv_bcast_packet *)skb->data;
> +
>  	/* check whether this has been sent by another originator before */
> -	if (batadv_bla_check_bcast_duplist(bat_priv, bcast_packet, hdr_size))
> +	if (batadv_bla_check_bcast_duplist(bat_priv, bcast_packet, skb->len))
>  		goto out;
>  
>  	/* rebroadcast packet */
> -- 
> 1.7.10.4
> 
>
  

On Wednesday, October 17, 2012 22:17:33 Simon Wunderlich wrote:
> Good catch! When can I buy you the beer? :D
> 
> Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

Applied in revision 2cebfac.

Thanks,
Marek
  

On Wednesday, October 17, 2012 22:17:05 Simon Wunderlich wrote:
> Looks good, although I don't think this will happen very often - it
> is only called when receiving packets, and AFAIK there can only be
> multiple threads when using multiple interfaces.
> 
> Anyway, doing the spinlock shouldn't hurt and might save us some pain
> when this function is used more often, or with multiple interfaces, or
> cases I don't see now. :)
> 
> Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

Applied in revision 4c1721b.

Thanks,
Marek