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

[Andrew Cooper]

On 21/11/2024 4:32 pm, Roger Pau Monné wrote:
> 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.coop...@citrix.com>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>

Thanks.

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

I'll adjust.

>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Frediano Ziglio <frediano.zig...@cloud.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Julien Grall <jul...@xen.org>
>>
>> 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?

For this, I'd prefer not to.

For ui, ul and ull, there are a pair of variables with precise usage.

This is one random number that gets as far as 2.  And it's test code.

~Andrew