[B.A.T.M.A.N.] [PATCH] batman-adv: make batadv_test_bit() return 0 or 1 only

[B.A.T.M.A.N.] [PATCH] batman-adv: make batadv_test_bit() return 0 or 1 only

[Linus Lüssing]

On some architectures test_bit() can return other values than 0 or 1:

With a generic x86 OpenWrt image in a kvm setup (batadv_)test_bit()
frequently returns -1 for me, leading to batadv_iv_ogm_update_seqnos()
wrongly signaling a protected seqno window.

This patch tries to fix this issue by making batadv_test_bit() return 0
or 1 only.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bitarray.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bitarray.h b/bitarray.h
index a081ce1..cebaae7 100644
--- a/bitarray.h
+++ b/bitarray.h
@@ -20,8 +20,8 @@
 #ifndef _NET_BATMAN_ADV_BITARRAY_H_
 #define _NET_BATMAN_ADV_BITARRAY_H_
 
-/* returns true if the corresponding bit in the given seq_bits indicates true
- * and curr_seqno is within range of last_seqno
+/* Returns 1 if the corresponding bit in the given seq_bits indicates true
+ * and curr_seqno is within range of last_seqno. Otherwise returns 0.
  */
 static inline int batadv_test_bit(const unsigned long *seq_bits,
 				  uint32_t last_seqno, uint32_t curr_seqno)
@@ -32,7 +32,7 @@ static inline int batadv_test_bit(const unsigned long *seq_bits,
 	if (diff < 0 || diff >= BATADV_TQ_LOCAL_WINDOW_SIZE)
 		return 0;
 	else
-		return  test_bit(diff, seq_bits);
+		return test_bit(diff, seq_bits) != 0;
 }
 
 /* turn corresponding bit on, so we can remember that we got the packet */
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: make batadv_test_bit() return 0 or 1 only

[Sven Eckelmann]

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

On Monday 03 September 2012 22:20:31 Linus Lüssing wrote:
> On some architectures test_bit() can return other values than 0 or 1:
> 
> With a generic x86 OpenWrt image in a kvm setup (batadv_)test_bit()
> frequently returns -1 for me, leading to batadv_iv_ogm_update_seqnos()
> wrongly signaling a protected seqno window.
> 
> This patch tries to fix this issue by making batadv_test_bit() return 0
> or 1 only.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Weird, I couldn't find the code that would cause a -1 here (but I just checked 
recent kernel versions). Most of them already added the "!= 0" in the function 
itself. And the both version of the variable_test_bit seem to return 0 and -1 
(at least my quick interpretation of the asm listings).

But you see it and therefore I would guess it is a real problem. Btw. this 
regression was introduced in a9f07211f65782d8f587ed3259ac8055b098cd24 (so it 
was in v2012.2.0 and 0079d2cef1514422668c7beedd61bfde5aa2c146 was part of 
linux 3.5)

Acked-by: Sven Eckelmann <sven@narfation.org>

Thanks

> ---
>  bitarray.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/bitarray.h b/bitarray.h
> index a081ce1..cebaae7 100644
> --- a/bitarray.h
> +++ b/bitarray.h
> @@ -20,8 +20,8 @@
>  #ifndef _NET_BATMAN_ADV_BITARRAY_H_
>  #define _NET_BATMAN_ADV_BITARRAY_H_
> 
> -/* returns true if the corresponding bit in the given seq_bits indicates
> true - * and curr_seqno is within range of last_seqno
> +/* Returns 1 if the corresponding bit in the given seq_bits indicates true
> + * and curr_seqno is within range of last_seqno. Otherwise returns 0.
>   */
>  static inline int batadv_test_bit(const unsigned long *seq_bits,
>  				  uint32_t last_seqno, uint32_t curr_seqno)
> @@ -32,7 +32,7 @@ static inline int batadv_test_bit(const unsigned long
> *seq_bits, if (diff < 0 || diff >= BATADV_TQ_LOCAL_WINDOW_SIZE)
>  		return 0;
>  	else
> -		return  test_bit(diff, seq_bits);
> +		return test_bit(diff, seq_bits) != 0;
>  }
> 
>  /* turn corresponding bit on, so we can remember that we got the packet */

[-- 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: make batadv_test_bit() return 0 or 1 only

[Sven Eckelmann]

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

On Monday 03 September 2012 23:11:39 Sven Eckelmann wrote:
> On Monday 03 September 2012 22:20:31 Linus Lüssing wrote:
> > On some architectures test_bit() can return other values than 0 or 1:
> > 
> > With a generic x86 OpenWrt image in a kvm setup (batadv_)test_bit()
> > frequently returns -1 for me, leading to batadv_iv_ogm_update_seqnos()
> > wrongly signaling a protected seqno window.
> > 
> > This patch tries to fix this issue by making batadv_test_bit() return 0
> > or 1 only.
> > 
> > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> 
> Weird, I couldn't find the code that would cause a -1 here (but I just
> checked recent kernel versions). Most of them already added the "!= 0" in
> the function itself. And the both version of the variable_test_bit seem to
> return 0 and -1 (at least my quick interpretation of the asm listings).

Just checked a asm reference and noticed that I remembered sbb wrong. The 
calculation behind it is: op2 - (op1 + CF).
I thought that it was: op2 - op1 + CF

[-- 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: make batadv_test_bit() return 0 or 1 only

[Sven Eckelmann]

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

On Monday 03 September 2012 23:13:41 Sven Eckelmann wrote:
[...]
> >And the both version of the variable_test_bit seem to
> > return 0 and -1 (at least my quick interpretation of the asm listings).
> 
> Just checked a asm reference and noticed that I remembered sbb wrong. The
> calculation behind it is: op2 - (op1 + CF).
> I thought that it was: op2 - op1 + CF

And I think I confused everyone right now. My original sentence should have 
said "seem to return 0 and 1" and not something about "-1".

The variable_test_bit for non-boot is implemented in asm and uses "btc" to 
check for the bit (that info is stored in CF as 0 for "not set" and 1 for 
"set"). sbb is used to store this information in the destination variable by 
substracting the original content of the variable from itself + also 
substracting the content of CF from it. That's why we get a -1 as result of 
test_bit on kvm.

Great finding and excellent patch.

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: make batadv_test_bit() return 0 or 1 only

[Marek Lindner]

On Tuesday, September 04, 2012 04:20:31 Linus Lüssing wrote:
> On some architectures test_bit() can return other values than 0 or 1:
> 
> With a generic x86 OpenWrt image in a kvm setup (batadv_)test_bit()
> frequently returns -1 for me, leading to batadv_iv_ogm_update_seqnos()
> wrongly signaling a protected seqno window.
> 
> This patch tries to fix this issue by making batadv_test_bit() return 0
> or 1 only.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  bitarray.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied in revision bf73b14.

Thanks,
Marek