Re: [PATCH] xen/bitops: Fix break with in a for_each_set_bit() loop

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Nov 21, 2024 at 2:50 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> for_each_set_bit()'s use of a double for loop had an accidental bug with a
> break in the inner loop leading to an infinite outer loop.
>
> Adjust for_each_set_bit() to avoid this behaviour, and add extend
> test_for_each_set_bit() with a test case for this.
>
> Fixes: ed26376f20bf ("xen/bitops: Introduce for_each_set_bit()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
>
> Both GCC and Clang seem happy with this, even at -O1:
>
>   https://godbolt.org/z/o6ohjrzsY
> ---
>  xen/common/bitops.c      | 16 ++++++++++++++++
>  xen/include/xen/bitops.h |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/bitops.c b/xen/common/bitops.c
> index 91ae961440af..0edd62d25c28 100644
> --- a/xen/common/bitops.c
> +++ b/xen/common/bitops.c
> @@ -110,6 +110,22 @@ static void __init test_for_each_set_bit(void)
>
>      if ( ull != ull_res )
>          panic("for_each_set_bit(uint64) expected %#"PRIx64", got 
> %#"PRIx64"\n", ull, ull_res);
> +
> +    /* Check that we break from the middle of the loop */
> +    ui = HIDE(0x80001008U);
> +    ui_res = 0;
> +    for_each_set_bit ( i, ui )
> +    {
> +        static __initdata unsigned int count;
> +
> +        if ( count++ > 1 )
> +            break;
> +
> +        ui_res |= 1U << i;
> +    }
> +
> +    if ( ui_res != 0x1008 )
> +        panic("for_each_set_bit(break) expected 0x1008, got %#x\n", ui_res);
>  }
>
>  static void __init test_multiple_bits_set(void)
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index 79615fb89d04..448b2d3e0937 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -299,7 +299,7 @@ static always_inline attr_const unsigned int 
> fls64(uint64_t x)
>   * A copy of @val is taken internally.
>   */
>  #define for_each_set_bit(iter, val)                     \
> -    for ( typeof(val) __v = (val); __v; )               \
> +    for ( typeof(val) __v = (val); __v; __v = 0 )       \
>          for ( unsigned int (iter);                      \
>                __v && ((iter) = ffs_g(__v) - 1, true);   \
>                __v &= __v - 1 )
>
> base-commit: e0058760a0c7935ad0690d8b9babb9050eceedf0

Not a fun of static variables but it's just in the test,

Reviewed-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

Frediano