Re: [BUG] LLC coloring: parse_color_config shorthand/UINT_MAX/partial configs + missing newline

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Mykola,

Replies inline below


On Tue, 13 Jan 2026, Mykola Kvach wrote:
> Hi all,
> 
> While investigating the Arm cache coloring, I noticed a few issues in
> parse_color_config() and one minor logging issue in the DT-related changes.
> 
> 1) parse_color_config() appears to accept "shorthand" forms not described
> by the documented grammar
> 
> The function documents:
> COLOR_CONFIGURATION ::= COLOR | RANGE,...,COLOR | RANGE
> RANGE ::= COLOR-COLOR
> 
> However, as implemented, inputs starting with a delimiter may be
> accepted and interpreted as if a leading 0 was present. For example:
> 
> ",2-6" -> interpreted as "0,2-6"
> "-10,2-6" -> interpreted as "0-10,2-6"
> 
> The reason is that the parser calls simple_strtoul(s, &s, 0) and then
> directly checks for '-'/',' at *s. If no digits were consumed, start
> becomes 0 and s can remain at the delimiter, which then gets treated as
> a range or separator.
> 
> Questions/proposed direction:
> - Is accepting these shorthand forms intentional? If yes, I think we
> should document them explicitly (both in the comment above
> parse_color_config() and in user-visible docs).
> - If not intentional, we likely want to reject by verifying that at
> least one digit was consumed for each number before accepting '-'
> or ','.

Yes, I think it was intentional. I would be happy with adding the
shorthands to the documentation.



> 2) Potential infinite loop/hang for ranges ending at UINT_MAX
> 
> The range expansion loop uses:
> for (color = start; color <= end; color++)
> 
> If end == UINT_MAX, incrementing color wraps back to 0, and
> (color <= end) remains true, resulting in an infinite loop inside early
> parsing. The current bounds checks do not prevent
> (start=UINT_MAX-some_small_number, end=UINT_MAX)
> from passing.

While that is true, no cache in existence can have even close to
UINT_MAX colors, and the number of colors are either passed by the user
(and assumed correct) or detected from the hardware.

While I wouldn't mind hardening the code against bad configurations and
I welcome a patch for it, we are talking about a patch that would turn a
crash into an explicit panic or similar. Also see below.


> 3) Partial global state on parse error can leak into later init/config use
> parse_color_config() writes into the provided array and increments
> *num_colors as it goes. On a parse error it returns -EINVAL, but the
> partially updated outputs remain.
> 
> his is particularly problematic for the cmdline parameters, because the
> utputs are global and can later be consumed by llc_coloring_init() or
> dom0_set_llc_colors().
> 
> A concrete example is "1,w,3-5":
> - "1" is parsed and committed
> - at "w", simple_strtoul() returns 0 and does not advance 's'
> - the code then treats this as a single color 0, commits it, and breaks
> out with *s != '\0'
> - the function returns -EINVAL, but num_colors and the array already
> contain a partial configuration
> 
> In other words, a rejected configuration can still leave xen_num_colors
> or dom0_num_colors non-zero and the corresponding colors[] partially
> populated, which risks the feature being initialized/applied with an
> unintended set of colors.
> 
> I think the parser should be fail-closed here. Minimally: reset
> *num_colors to 0 on any error path (including the final "*s ? -EINVAL"
> case). Ideally: parse transactionally into a temporary buffer and only
> commit outputs on full success.

I think we should panic on parsing errors: cache coloring is not a
feature for beginners that a distro like Debian is going to enable to
improve the out of the box experience with new users. Cache coloring is
a sophisticated feature that takes time to learn how to use it
effectively. If an experience embedded developer is providing an
erroneous configuration the best we can do it panic so that it becomes
immediately obvious what they did wrong and they can fix it.


> 4) Cosmetic: missing newline in printk in domain_set_llc_colors_from_str()
> 
> printk(XENLOG_ERR "Error parsing LLC color configuration");
> without a trailing '\n'.
> 
> I wanted to flag these issues in case you’d like to address them in the
> next revision/follow-up.

Sure please fix