Re: [PATCH v14 12/13] virt/steal_governor: Implement steal_governor policy loop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing bounds check on interval_ms allows infinite workqueue spinloop
- [High] TOCTOU data race in preferred_cpus_valid() causes erroneous driver shutdown
- [Medium] Integer overflow in steal_ratio denominator when VM is paused
--

commit 5f38399332407b64e0ce2da43681c27f1e1287d5
Author: Shrikanth Hegde <sshegde@linux.ibm.com>

virt/steal_governor: Implement steal_governor policy loop

Schedule work at regular intervals to implement the steal_governor
policy loop, which monitors steal time and takes action on the state of
preferred CPUs. The interval is determined by the interval_ms parameter.

> diff --git a/drivers/virt/steal_governor.c b/drivers/virt/steal_governor.c
> index 27f53ea16498b..6e31f9923dea4 100644
> --- a/drivers/virt/steal_governor.c
> +++ b/drivers/virt/steal_governor.c
[ ... ]
> +static bool preferred_cpus_valid(void)
> +{
> +	if (cpumask_empty(cpu_preferred_mask)) {
> +		pr_err("empty preferred mask. stopping\n");
> +		return false;
> +	}
> +
> +	if (!cpumask_subset(cpu_preferred_mask, cpu_active_mask)) {

[Severity: High]
Does this lockless read of cpu_preferred_mask and cpu_active_mask race
with CPU hotplug?

Since CPU hotplug clears the preferred mask bit before the active mask bit,
it seems cpumask_subset() can read a torn state where a word is read from
the preferred mask before the bit is cleared, and the corresponding word
is read from the active mask after the bit is cleared, returning false.

> +		pr_err("preferred: %*pbl is not subset of active: %*pbl, stopping\n",
> +		       cpumask_pr_args(cpu_preferred_mask),
> +		       cpumask_pr_args(cpu_active_mask));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void steal_governor_loop(struct work_struct *work)
> +{
> +	u64 curr_steal, delta_steal, delta_ns, steal_ratio;
> +	ktime_t now;
> +
> +	now = ktime_get();
> +	delta_ns = ktime_to_ns(ktime_sub(now, sg_ctx.time));
> +
> +	if (unlikely(delta_ns < NSEC_PER_MSEC)) {
> +		pr_err_ratelimited("work scheduled too soon delta_ns: %llu\n", delta_ns);
> +		goto requeue_work;

[Severity: High]
Can this create an infinite spinloop if interval_ms is set to 0?

If interval_ms is 0, the delay is 0 jiffies. When the work executes, it will
take this branch because delta_ns < 1ms. It jumps directly to requeue_work
without updating sg_ctx.time, which schedules it again with 0 delay. This
will continuously spin and consume an entire CPU core.

> +	}
> +
> +	curr_steal = get_system_steal_time();
> +	delta_steal = curr_steal > sg_ctx.steal ? curr_steal - sg_ctx.steal : 0;
> +	sg_ctx.steal = curr_steal;
> +	sg_ctx.time = now;
> +
> +	/*
> +	 * steal_ratio = (delta_steal * 100*100)/(delta_ns * num_cpus())
> +	 * To avoid possible overflow, divide the denominator early.
> +	 * Note minimum interval is 100ms.
> +	 */
> +	delta_ns = max_t(u64, div_u64(delta_ns * get_system_cpus(), 10000), 1);

[Severity: Medium]
Will this silently overflow the 64-bit multiplication if the VM is paused
for an extended period?

Since the multiplication of delta_ns * get_system_cpus() happens before
the division, a large time delta (e.g., a large VM suspended for a long time)
can overflow U64_MAX, leading to a wildly incorrect steal_ratio calculation.

> +	steal_ratio = div64_u64(delta_steal, delta_ns);
> +
> +	if (steal_ratio > sg_ctx.high_threshold)
> +		decrease_preferred_cpus();
> +	else if (steal_ratio <= sg_ctx.low_threshold)
> +		increase_preferred_cpus();
> +	/*
> +	 * else: steal ratio is within bounds. Still do design checks so that
> +	 * module restores to active if CPU hotplug breaks those assumptions.
> +	 */
> +	if (!preferred_cpus_valid()) {

[Severity: High]
Does calling preferred_cpus_valid() here without cpus_read_lock() lead
to a false teardown?

If a hotplug operation is happening concurrently, the torn state read in
preferred_cpus_valid() will cause the governor to erroneously shut down
the policy loop permanently.

> +		restore_preferred_to_active();
> +		return;
> +	}
> +
> +requeue_work:
> +	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);

[Severity: High]
Does this immediately spin if sg_ctx.delay is 0, as mentioned above?

> +}
> +
>  static int __init steal_governor_init(void)
>  {
[ ... ]
>  	sg_ctx.delay = msecs_to_jiffies(sg_ctx.interval_ms);

[Severity: High]
Is it possible for sg_ctx.interval_ms to be 0 here?

The module parameter interval_ms doesn't appear to be bounds-checked
before this point, meaning it could be set to 0 and trigger the infinite
workqueue spinloop.

> +	INIT_DELAYED_WORK(&sg_ctx.work, steal_governor_loop);
> +	sg_ctx.steal = get_system_steal_time();
> +	sg_ctx.time = ktime_get();
> +	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
>  	pr_info("enabled. interval: %ums, high_threshold: %u, low_threshold: %u\n",
>  		sg_ctx.interval_ms, sg_ctx.high_threshold, sg_ctx.low_threshold);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928053728.797539-1-sshegde@linux.ibm.com?part=12