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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 21, 2024 at 02:50:00PM +0000, Andrew Cooper 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>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have to admit it took me a while to understand what was going on.

Subject should likely be "Fix break usage in for_each_set_bit() loop"

Or similar?

> ---
> 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;

Preferably as you suggested without the static variable, I may suggest
that you use ui_tmp instead of plain tmp as the variable name?

Thanks, Roger.