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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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