Re: [PATCH 9/9] x86/mwait-idle: Add C-states validation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 12, 2026 at 05:58:21PM +0100, Jan Beulich wrote:
> From: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> 
> Add validation for C-states specified via the "table=" module parameter.
> Treat this module parameter as untrusted input and validate it thoroughly.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> Link: https://patch.msgid.link/20251216080402.156988-4-dedekind1@xxxxxxxxx
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> be6a150829b3
> 
> Add __init to validate_cmdline_cstate(). Other adjustments to fit our env.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -72,6 +72,11 @@ boolean_param("mwait-idle", opt_mwait_id
>  
>  /* The maximum allowed length for the 'table' module parameter  */
>  #define MAX_CMDLINE_TABLE_LEN 256
> +/* Maximum allowed C-state latency */
> +#define MAX_CMDLINE_LATENCY_US (5 * 1000 /* USEC_PER_MSEC */)
> +/* Maximum allowed C-state target residency */
> +#define MAX_CMDLINE_RESIDENCY_US (100 * 1000 /* USEC_PER_MSEC */)
> +
>  static char cmdline_table_str[MAX_CMDLINE_TABLE_LEN] __initdata;
>  string_param("mwait-idle.table", cmdline_table_str);
>  
> @@ -1589,6 +1594,41 @@ static char *__init get_cmdline_field(ch
>  }
>  
>  /**
> + * validate_cmdline_cstate - Validate a C-state from cmdline.
> + * @state: The C-state to validate.
> + * @prev_state: The previous C-state in the table or NULL.
> + *
> + * Return: 0 if the C-state is valid or -EINVAL otherwise.

Hm, I know we picked this up from upstream, but this function would
better return a boolean, rather than 0 or -EINVAL.

> + */
> +static int __init validate_cmdline_cstate(struct cpuidle_state *state,
> +                                       struct cpuidle_state *prev_state)
> +{
> +     if (state->exit_latency == 0)
> +             /* Exit latency 0 can only be used for the POLL state */
> +             return -EINVAL;
> +
> +     if (state->exit_latency > MAX_CMDLINE_LATENCY_US)
> +             return -EINVAL;
> +
> +     if (state->target_residency > MAX_CMDLINE_RESIDENCY_US)
> +             return -EINVAL;
> +
> +     if (state->target_residency < state->exit_latency)
> +             return -EINVAL;
> +
> +     if (!prev_state)
> +             return 0;
> +
> +     if (state->exit_latency <= prev_state->exit_latency)
> +             return -EINVAL;
> +
> +     if (state->target_residency <= prev_state->target_residency)
> +             return -EINVAL;

I'm not an expert on C-states, but isn't this checking against the
previous value kind of defeating part of the purpose of the command
line?

Also, it might help to also write down those limits in the command
line documentation.

Thanks, Roger.