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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

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 ','.

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.

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.

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.


Thanks,
Mykola