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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 10 Apr 2026, at 08:04, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
> 
> 
> 
> On 10/04/2026 08:57, Jan Beulich wrote:
>> 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?
> Yes, I do agree. This patch can be modified to just do:
> 
> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> index eb7c72b24023..6dc614739a98 100644
> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, 
> unsigned
> int colors[],
> 
>         if ( end >= NR_LLC_COLORS || start > end ||
>              (end - start) >= (UINT_MAX - *num_colors) ||
> -             (*num_colors + (end - start + 1)) >= max_num_colors )
> +             (*num_colors + (end - start)) >= max_num_colors )
>             return -EINVAL;
> 
>         /* Colors are range checked in check_colors() */
> 
> I'll do that later on.
> 
> ~Michal
> 

Feel free to keep my R-by for this change.

Cheers,
Luca