linux-kernel - Re: [PATCH v2] cpuidle: Add sanity check for exit latency and target residency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0hgjZdRTbDnTz7frt5+Grrt0Dft_TJgW0-t92XupCbzXw@mail.gmail.com>
Date: Sat, 8 Nov 2025 12:02:42 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>
Subject: Re: [PATCH v2] cpuidle: Add sanity check for exit latency and target residency

On Sat, Nov 8, 2025 at 9:49 AM Artem Bityutskiy
<artem.bityutskiy@...ux.intel.com> wrote:
>
> On Fri, 2025-11-07 at 20:07 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Make __cpuidle_driver_init() fail if the exit latency of one of the
> > driver's idle states is less than its exit latency which would break
> > cpuidle assumptions.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> LGTM
>
> Reviewed-by: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
>
>
> By the way, I have a more paranoid validation patch, which validates
> latency and also more. Sure I can rebase it later on top of this
> patch.

That should be rather straightforward.

> I did not have time to polish it yet, but sharing just in case there is
> a quick feedback.
>
> From: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
> Subject: [PATCH] cpuidle: Add idle states validation
>
> Validate the idle states table provided by the underlying idle driver. For
> example, validate that deeper idle states have greater latency and target
> residency compared to shallower states.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
> ---
>  drivers/cpuidle/driver.c | 58 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 9bbfa594c4425..6bcedad534dd9 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -20,6 +20,10 @@
>
>  #include "cpuidle.h"
>
> +/* Maximum allowed latency and target residency values */
> +#define MAX_LATENCY 50000 /* 50 milliseconds */
> +#define MAX_RESIDENCY 1000000 /* 1 second */
> +
>  DEFINE_SPINLOCK(cpuidle_driver_lock);
>
>  #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> @@ -148,11 +152,46 @@ static void cpuidle_setup_broadcast_timer(void *arg)
>                 tick_broadcast_disable();
>  }
>
> +/**
> + * validate_state - Validate an idle state.
> + * @state: The C-state to validate.
> + * @prev_state: The previous idle state or NULL.
> + *
> + * Return: 0 if the idle state is valid or -EINVAL otherwise.
> + */
> +static int validate_state(struct cpuidle_state *s, struct cpuidle_state *prev_s)
> +{
> +       if (s->exit_latency == 0)
> +               return -EINVAL;

The change above will break the polling state AFAICS.

> +
> +       if (s->exit_latency > MAX_LATENCY)
> +               return -EINVAL;
> +
> +       if (s->target_residency > MAX_RESIDENCY)
> +               return -EINVAL;
> +
> +       if (s->target_residency < s->exit_latency)
> +               return -EINVAL;
> +
> +       if (!prev_s)
> +               return 0;
> +
> +       if (s->exit_latency <= prev_s->exit_latency)
> +               return -EINVAL;

Well, is this really necessary?  Nothing depends on this ordering AFAICS.

> +
> +       if (s->target_residency <= prev_s->target_residency)
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
>  /**
>   * __cpuidle_driver_init - initialize the driver's internal data
>   * @drv: a valid pointer to a struct cpuidle_driver
> + *
> + * Return: 0 on success, -EINVAL in case of invalid C-state data.
>   */
> -static void __cpuidle_driver_init(struct cpuidle_driver *drv)
> +static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>  {
>         int i;
>
> @@ -166,6 +205,17 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>
>         for (i = 0; i < drv->state_count; i++) {
>                 struct cpuidle_state *s = &drv->states[i];
> +               struct cpuidle_state *prev_s;
> +
> +               if (i > 0)
> +                       prev_s = &drv->states[i - 1];
> +               else
> +                       prev_s = NULL;
> +
> +               if (validate_state(s, prev_s)) {

I would use the ternary operator here:

              if (validate_state(s, i ? &drv->states[i - 1] : NULL)) {

or just pass drv and i to validate_state().

But this needs to be done when all of the values are known. ->

> +                       pr_err("Idle state '%s' validation failed\n", s->name);
> +                       return -EINVAL;
> +               }
>
>                 /*
>                  * Look for the timer stop flag in the different states and if
> @@ -194,6 +244,8 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
>                 else
>                         s->exit_latency = div_u64(s->exit_latency_ns, NSEC_PER_USEC);

-> Which is here.

>         }
> +
> +       return 0;
>  }
>
>  /**
> @@ -223,7 +275,9 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
>         if (cpuidle_disabled())
>                 return -ENODEV;
>
> -       __cpuidle_driver_init(drv);
> +       ret = __cpuidle_driver_init(drv);
> +       if (ret)
> +               return ret;
>
>         ret = __cpuidle_set_driver(drv);
>         if (ret)
> --