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, [email protected] 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 <[email protected]>
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);
