Re: [PATCH 2/5] llc-coloring: improve checking while parsing

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/04/2026 11:37 am, Jan Beulich wrote:
> On 07.04.2026 12:30, Andrew Cooper wrote:
>> On 24/03/2026 4:37 pm, Jan Beulich wrote:
>>> We can easily avoid the risk of wrapping UINT_MAX <-> 0 by applying a
>>> check against the compile-time-constant maximum number of colors.
>>>
>>> Additionally the overflow checks suffered from an off-by-1, as the parsed
>>> ranges are inclusive (e.g. end == start being possible, requiring 1 array
>>> slot, while availability of 0 slots was checked in that case).
>>>
>>> Fixes: 6cdea3444eaf ("xen/arm: add Dom0 cache coloring support")
>>> Reported-by: Kamil Frankowicz <kamil.frankowicz@xxxxxxx>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> --- a/xen/common/llc-coloring.c
>>> +++ b/xen/common/llc-coloring.c
>>> @@ -76,8 +76,9 @@ static int __init parse_color_config(con
>>>          else                /* Single value */
>>>              end = start;
>>>  
>>> -        if ( start > end || (end - start) > (UINT_MAX - *num_colors) ||
>>> -             (*num_colors + (end - start)) >= max_num_colors )
>>> +        if ( end >= NR_LLC_COLORS || start > end ||
>>> +             (end - start) >= (UINT_MAX - *num_colors) ||
>>> +             (*num_colors + (end - start + 1)) >= max_num_colors )
>>>              return -EINVAL;
>>>  
>>>          /* Colors are range checked in check_colors() */
>>>
>> I think this is correct, so Acked-by: Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>
> Thanks.
>
>> However, the parsing logic is also ridiculous.  Most of the complexity
>> comes because of parsing a bitmap but storing it longhand in an array of
>> unsigned ints.
>>
>> Instead, the global variables default_colors, dom0_colors and xen_colors
>> should be bitmaps sized by NR_LLC_COLORS, and d->llc_colours should be a
>> bitmap sized by xen_num_colors (which itself is bound by NR_LLC_COLORS).
>>
>> With the default of 32 colours, this would involve no memory allocation
>> at all, even on 32bit builds of Xen.
> It's pretty space inefficient, yes, but the parsing wouldn't become simpler
> when using bitmaps, would it?

Yes it would.

The current logic has opencoded bitmap helpers, and removing *num_colors
simplifies all the boundary conditions (you simply don't need a cursor
when collecting into a real bitmap.)

~Andrew