Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.04.2026 15:34, Luca Fancellu wrote:
>> On 9 Apr 2026, at 13:52, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>>> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 09.04.2026 14:22, Luca Fancellu wrote:
>>>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>>>>
>>>>> The check uses >= to compare the total number of colors against
>>>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>>>> incorrectly rejects input that would exactly fill the array.
>>>>>
>>>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>>>> for dom0 would fail.
>>>>>
>>>>> Change >= to > so that exactly filling the array is permitted.
>>>>>
>>>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>>> ---
>>>>
>>>> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>>
>>> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
>>> think a 2nd Fixes: tag may be needed here. For now I can't help the
>>> impression though that there might have been a re-basing mistake, where
>>> that re-base may have wanted to result in this patch dissolving into
>>> nothing. Yet of course I'm all ears to learn otherwise.
>>
>> Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer 
>> look.
> 
> 
> I had a closer look, I feel that the patch is ok and commit 
> cba8a584de171c8c4510709c2edc9f1cf86b21ab
> was missing this corner case.

If anything, that part of the change there was outright wrong (and hence, as
said, a 2nd Fixes: tag [actually, see below, simply another one] is needed).
With overflow excluded,

    (*num_colors + (end - start + 1)) > max_num_colors

is the same as

   (*num_colors + (end - start)) >= max_num_colors

i.e. the state before that change, isn't it?

And yes, now that I look again I think I agree that I screwed up there. Yet
then the (imo) better fix would be to undo that change, rather than switching
from >= to > . That's one less calculation overall. Michal?

Jan

> Let’s say max_num_colors = 8 (array capacity), *num_colors = 4 so we stored 
> already 4 entries and the
> next parsed range gives start = 4, end = 7: 
> 
> (*num_colors + (end - start + 1)) >= max_num_colors will compute as
> (4 + (7 - 4 + 1)) >= 8 which will be
> 8 >= 8 that will be true and the input will be rejected, instead of being a 
> valid entry.
> 
> Did I miss anything?
> 
> Cheers,
> Luca
>