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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

I don't see any real concerns so far. Some optimizations yes, but they
can be deferred post the merge if they are necessary.

On 9/28/26 11:21 AM, sashiko-bot@kernel.org wrote:
> 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.

Update ordering is taken care. Not an issue is clear/set bit involves atomics.

> 
>> +		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.
> 

No. Interval can't be set to 0. Minimum is 100ms.

>> +	}
>> +
>> +	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.
> 

Not a reason to fret about. It is way too long duration.

>> +	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?

As said above, interval cannot be less than 100ms.

> 
>> +}
>> +
>>   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.

As said above, interval cannot be less than 100ms.

> 
>> +	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);
>>
>