Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
> On 10 Apr 2026, at 08:04, Orzel, Michal 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 wrote:
> On 9 Apr 2026, at 13:48, Jan Beulich wrote:
> On 09.04.2026 14:22, Luca Fancellu wrote:
>>> On 9 Apr 2026, at 12:39, Michal Orzel 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
>>> ---
>>
>> Reviewed-by: Luca Fancellu
>
> 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
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
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 wrote:
On 9 Apr 2026, at 13:48, Jan Beulich wrote:
On 09.04.2026 14:22, Luca Fancellu wrote:
>> On 9 Apr 2026, at 12:39, Michal Orzel 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
>> ---
>
> Reviewed-by: Luca Fancellu
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
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
On 09.04.2026 15:34, Luca Fancellu wrote:
>> On 9 Apr 2026, at 13:52, Luca Fancellu wrote:
>>> On 9 Apr 2026, at 13:48, Jan Beulich wrote:
>>> On 09.04.2026 14:22, Luca Fancellu wrote:
> On 9 Apr 2026, at 12:39, Michal Orzel 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
> ---
Reviewed-by: Luca Fancellu
>>>
>>> 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
>
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
Hi Jan,
> On 9 Apr 2026, at 13:52, Luca Fancellu wrote:
>
>
>
>> On 9 Apr 2026, at 13:48, Jan Beulich wrote:
>>
>> On 09.04.2026 14:22, Luca Fancellu wrote:
On 9 Apr 2026, at 12:39, Michal Orzel 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
---
>>>
>>> Reviewed-by: Luca Fancellu
>>
>> 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.
>>
>> Jan
>
> 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.
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
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
> On 9 Apr 2026, at 13:48, Jan Beulich wrote:
>
> On 09.04.2026 14:22, Luca Fancellu wrote:
>>> On 9 Apr 2026, at 12:39, Michal Orzel 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
>>> ---
>>
>> Reviewed-by: Luca Fancellu
>
> 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.
>
> Jan
Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer
look.
Cheers,
Luca
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
On 09/04/2026 13:47, Andrew Cooper wrote:
> On 09/04/2026 12:39 pm, Michal Orzel 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
>> ---
>> xen/common/llc-coloring.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
>> index eb7c72b24023..30c1594dac9f 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 + 1)) > max_num_colors )
>> return -EINVAL;
>>
>> /* Colors are range checked in check_colors() */
>
> This boundary was changed by
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cba8a584de171c8c4510709c2edc9f1cf86b21ab
> because it was off-by-one.
>
> Are you saying that the analysis in that patch was wrong?
I examined the scenario that is a default for dom0 i.e. dom0 gets all the colors
by default. This is equivalent to setting dom0-llc-colors=0-15. If I set this, I
will get a message:
(XEN) parameter "dom0-llc-colors" has invalid value "0-15", rc=-22!
I admit that I added wrong example in commit msg.
~Michal
>
> ~Andrew
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
On 09.04.2026 14:22, Luca Fancellu wrote:
>> On 9 Apr 2026, at 12:39, Michal Orzel 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
>> ---
>
> Reviewed-by: Luca Fancellu
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.
Jan
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
Hi Michal,
> On 9 Apr 2026, at 12:39, Michal Orzel 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
> ---
Reviewed-by: Luca Fancellu
Cheers,
Luca
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
On 09/04/2026 12:39, Michal Orzel 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. This seems related to BVA as well. For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15 for dom0 would fail. Why does it fail ? Is it because the max number of colors can be 15. - Ayan
Re: [PATCH 2/3] xen/common: llc-coloring: Fix off-by-one in parse_color_config()
On 09/04/2026 12:39 pm, Michal Orzel 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
> ---
> xen/common/llc-coloring.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> index eb7c72b24023..30c1594dac9f 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 + 1)) > max_num_colors )
> return -EINVAL;
>
> /* Colors are range checked in check_colors() */
This boundary was changed by
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cba8a584de171c8c4510709c2edc9f1cf86b21ab
because it was off-by-one.
Are you saying that the analysis in that patch was wrong?
~Andrew
