[B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[Linus Lüssing]

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 regrission introduced by
e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.

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)

 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 6705d35..e7b5777 100644
--- a/bridge_loop_avoidance.c
+++ b/bridge_loop_avoidance.c
@@ -1205,8 +1205,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,
@@ -1219,14 +1219,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 bc2b88b..f861b7c 100644
--- a/routing.c
+++ b/routing.c
@@ -1136,8 +1136,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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[Antonio Quartulli]

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

On Wed, Oct 17, 2012 at 01:38:30PM +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 regrission introduced by
> e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Hi Linus,

nice catch!! Don't know how much time you spent to find the correct point where
your broadcast packets were getting lost.. :-)


Is it ok for you if we change the commit hash to ("batman-adv: add broadcast
duplicate check") when merging this patch? Lately Dave complained about commit
hashes in patch because it may happen that a developer uses a different hash
function for his git repo.

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: Fix broadcast packet CRC calculation

[Gui Iribarren]

[resending as plaintext since maillist refused to accept apparently a
Content-Type: text/html]

On Wed, Oct 17, 2012 at 9:42 AM, Gui Iribarren <gui@altermundi.net> wrote:
Ah, interesting! Does this affect multi-hop broadcasts as well? Maybe
this could be somehow related to the misteriously dropped avahi
packets we experience here. Nico Echaniz reported the case in a
previous thread[0] but so far we didn't have time to debug it further.
If this patch affects anything described in that mail (a couple of
months ago) we are eager to test it out :)

[0] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2012-July/007763.html

On Wed, Oct 17, 2012 at 8:56 AM, Antonio Quartulli <ordex@autistici.org> wrote:
> On Wed, Oct 17, 2012 at 01:38:30PM +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 regrission introduced by
>> e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.
>>
>> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
>
> Hi Linus,
>
> nice catch!! Don't know how much time you spent to find the correct point where
> your broadcast packets were getting lost.. :-)
>
>
> Is it ok for you if we change the commit hash to ("batman-adv: add broadcast
> duplicate check") when merging this patch? Lately Dave complained about commit
> hashes in patch because it may happen that a developer uses a different hash
> function for his git repo.
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[Sven Eckelmann]

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

On Wednesday 17 October 2012 13:38:30 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 regrission introduced by
> e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.
> 
> 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)
> 
>  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 6705d35..e7b5777 100644
> --- a/bridge_loop_avoidance.c
> +++ b/bridge_loop_avoidance.c
> @@ -1205,8 +1205,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,
> @@ -1219,14 +1219,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 bc2b88b..f861b7c 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -1136,8 +1136,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;


Please don't ues linearize here. Try to fix the crc calculation using 
skb_walk_frags (see for example net/netfilter/nf_nat_proto_sctp.c ).

And Simon owes you a beer for this debugging ;)

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[Sven Eckelmann]

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

On Wednesday 17 October 2012 13:38:30 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 regrission introduced by
> e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.
> 
> 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)
> 
>  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 6705d35..e7b5777 100644
> --- a/bridge_loop_avoidance.c
> +++ b/bridge_loop_avoidance.c
> @@ -1205,8 +1205,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,
> @@ -1219,14 +1219,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 bc2b88b..f861b7c 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -1136,8 +1136,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 */

Btw. why should renaming the hdr_size to bcast_packet_len help to get a 
different value than 0?

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Fix broadcast packet CRC calculation

[Sven Eckelmann]

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

On Wednesday 17 October 2012 15:50:50 Sven Eckelmann wrote:
> On Wednesday 17 October 2012 13:38:30 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 regrission introduced by
> > e5cf86d30a9b32feccb6b866a3f23b0e0483a3b0.
> > 
> > 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)
> > 
> >  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 6705d35..e7b5777 100644
> > --- a/bridge_loop_avoidance.c
> > +++ b/bridge_loop_avoidance.c
> > @@ -1205,8 +1205,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,
> > 
> > @@ -1219,14 +1219,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 bc2b88b..f861b7c 100644
> > --- a/routing.c
> > +++ b/routing.c
> > @@ -1136,8 +1136,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 */
> 
> Btw. why should renaming the hdr_size to bcast_packet_len help to get a
> different value than 0?

Ok, Marek just noticed the skb->len :D

> Kind regards,
> 	Sven

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