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. > + 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/[email protected]?part=12
