batman-adv: make batadv_test_bit() return 0 or 1 only
Commit Message
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(-)
Comments
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 */
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
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
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
@@ -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 */