From ada847daceb632260ca9b1d029207e231049f308 Mon Sep 17 00:00:00 2001 From: John Starks Date: Thu, 30 Jul 2026 04:12:49 -0700 Subject: [PATCH] drivers/hv/mshv_vtl: fix fabricated sidecar VP run completions sidecar_scan_vps() treats cpu_status == CPU_STATUS_IDLE together with vp_state == VP_STATE_ASYNC as an async run that has completed. But a VP is equally idle before its run has been started, so that pair only means "completed" if the IDLE was observed after the start. sidecar_ioctl_start() published the claim first: sidecar_claim(dev, cpu, VP_STATE_ASYNC); sidecar_start(dev, cpu); Between those two statements the VP is idle but owned, so a concurrent scan reports a completion for a run that never happened. Userspace then consumes a command page the AP never wrote, replaying the previous intercept message against register state it has already emulated. This shows up as a mismatch between the rax in the intercept message and the VP's register state on the first IO port exit after the VP is moved back to Linux. Claim into a new VP_STATE_ASYNC_STARTING, which the scan does not accept, and publish VP_STATE_ASYNC only once sidecar_start() has set cpu_status to CPU_STATUS_RUN. Publish with xchg() so the store is ordered against both the preceding cmpxchg() and the cpu_status load that follows; a release store would let that load be hoisted, losing the re-arm described below. Invert the load order in the scan to match, so that observing VP_STATE_ASYNC implies observing CPU_STATUS_RUN. The AP can run and go idle while the state is still VP_STATE_ASYNC_STARTING, in which case a scan in that window skips the VP and consumes its attention. Re-arm the scan after publishing so the completion is not lost. Auditing the surrounding completion-reporting path turned up three more defects, fixed here as well. None of them is required to close the race above, so the ASYNC_STARTING change alone is self-contained if a minimal backport is wanted. First, move the locking out of the scan and into its callers, renaming it sidecar_scan_vps_locked(). The claim loop in sidecar_scan_next_stopped() ran outside scan_mutex, so two readers could claim the same VP_STATE_ASYNC_STOPPED entry, report the same CPU twice and underflow num_vps_stopped, after which its zero fast path never short-circuits again. Holding the mutex across both the scan and the claim makes the counter exact and removes the need to update it atomically. Second, wake the wait queue from whichever path marks entries stopped, after the counter has been incremented. sidecar_scan_vps_locked() consumes needs_vp_scan, but sidecar_poll() never claims the entries it marks, so a reader woken by the ISR that set needs_vp_scan could find both that flag and num_vps_stopped clear and go back to sleep with a completion pending. Third, return the pending count from sidecar_scan_vps_locked() instead of having both callers re-read num_vps_stopped under the same lock. The early return has to report the count as well: when needs_vp_scan is clear the scan does nothing, but entries marked by an earlier scan may still be waiting to be claimed. Signed-off-by: John Starks --- drivers/hv/mshv_vtl_sidecar.c | 83 +++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 22 deletions(-) diff --git a/drivers/hv/mshv_vtl_sidecar.c b/drivers/hv/mshv_vtl_sidecar.c index 1347ad51fa637..f5d8be2c94de7 100644 --- a/drivers/hv/mshv_vtl_sidecar.c +++ b/drivers/hv/mshv_vtl_sidecar.c @@ -61,6 +61,8 @@ DEFINE_PER_CPU(struct sidecar_dev *, sidecar_interrupt_dev); #define VP_STATE_ASYNC 2 #define VP_STATE_ASYNC_STOPPING 3 #define VP_STATE_ASYNC_STOPPED 4 +/* Claimed for an async run, but the AP has not been handed the command yet. */ +#define VP_STATE_ASYNC_STARTING 5 #define VP_STATE_REMOVED 0xff static void mshv_vtl_sidecar_isr(void) @@ -161,54 +163,73 @@ static int sidecar_remove(unsigned int cpu) } -static void sidecar_scan_vps(struct sidecar_dev *dev) +/* Returns the number of VPs with a completion waiting to be reported. */ +static u32 sidecar_scan_vps_locked(struct sidecar_dev *dev) { + bool stopped = false; u32 count; u32 i; u8 *slot; u8 state; + lockdep_assert_held(&dev->scan_mutex); + if (!READ_ONCE(dev->needs_vp_scan)) - return; + return dev->num_vps_stopped; xchg(&dev->needs_vp_scan, 0); - mutex_lock(&dev->scan_mutex); count = dev->cpu_count; for (i = 0; i < count; i++) { + /* + * Load vp_state first: ASYNC is published only after + * cpu_status is RUN, so a later IDLE is a real completion. + */ + state = smp_load_acquire(&dev->vp_state[i]); + if (state != VP_STATE_ASYNC_STOPPING && state != VP_STATE_ASYNC) + continue; slot = &dev->control->cpu_status[i]; if (READ_ONCE(*slot) != CPU_STATUS_IDLE) continue; - state = READ_ONCE(dev->vp_state[i]); - if (state != VP_STATE_ASYNC_STOPPING && state != VP_STATE_ASYNC) - continue; WRITE_ONCE(dev->vp_state[i], VP_STATE_ASYNC_STOPPED); - xadd(&dev->num_vps_stopped, 1); + WRITE_ONCE(dev->num_vps_stopped, dev->num_vps_stopped + 1); + stopped = true; } - mutex_unlock(&dev->scan_mutex); + + /* + * The scan consumed needs_vp_scan, so a reader woken by the attention + * that set it may already have found both it and the count clear. + */ + if (stopped) + wake_up_poll(&dev->wait, EPOLLIN); + + return dev->num_vps_stopped; } static int sidecar_scan_next_stopped(struct sidecar_dev *dev) { u32 count; u32 i; - u8 state; + int ret = -1; - sidecar_scan_vps(dev); - if (READ_ONCE(dev->num_vps_stopped) == 0) - return -1; + mutex_lock(&dev->scan_mutex); + if (!sidecar_scan_vps_locked(dev)) + goto out; count = dev->cpu_count; for (i = 0; i < count; i++) { - state = dev->vp_state[i]; - if (state == VP_STATE_ASYNC_STOPPED) { - xadd(&dev->num_vps_stopped, -1); - WRITE_ONCE(dev->vp_state[i], VP_STATE_AVAIL); - return dev->base_cpu + i; - } + if (READ_ONCE(dev->vp_state[i]) != VP_STATE_ASYNC_STOPPED) + continue; + + WRITE_ONCE(dev->vp_state[i], VP_STATE_AVAIL); + WRITE_ONCE(dev->num_vps_stopped, dev->num_vps_stopped - 1); + ret = dev->base_cpu + i; + break; } - return -1; +out: + mutex_unlock(&dev->scan_mutex); + return ret; } static __poll_t sidecar_poll(struct file *filp, poll_table *wait) @@ -218,9 +239,11 @@ static __poll_t sidecar_poll(struct file *filp, poll_table *wait) dev = filp->private_data; poll_wait(filp, &dev->wait, wait); - sidecar_scan_vps(dev); - if (READ_ONCE(dev->num_vps_stopped) > 0) + + mutex_lock(&dev->scan_mutex); + if (sidecar_scan_vps_locked(dev)) mask |= EPOLLIN | EPOLLRDNORM; + mutex_unlock(&dev->scan_mutex); return mask; } @@ -279,13 +302,28 @@ static int sidecar_ioctl_run(struct sidecar_dev *dev, u32 cpu) static int sidecar_ioctl_start(struct sidecar_dev *dev, u32 cpu) { + u32 cpu_index = cpu - dev->base_cpu; int ret; - ret = sidecar_claim(dev, cpu, VP_STATE_ASYNC); + /* The scan ignores this state, hiding the claim/start window from it. */ + ret = sidecar_claim(dev, cpu, VP_STATE_ASYNC_STARTING); if (ret) return ret; sidecar_start(dev, cpu); + + /* Fully ordered: publishes the start and orders the load below. */ + xchg(&dev->vp_state[cpu_index], VP_STATE_ASYNC); + + /* + * The AP may have completed while the state was ASYNC_STARTING; a scan + * in that window skipped it and consumed its attention. + */ + if (READ_ONCE(dev->control->cpu_status[cpu_index]) != CPU_STATUS_RUN) { + WRITE_ONCE(dev->needs_vp_scan, 1); + wake_up_poll(&dev->wait, EPOLLIN); + } + return 0; } @@ -304,6 +342,7 @@ static int sidecar_ioctl_stop(struct sidecar_dev *dev, u32 cpu) switch (state) { case VP_STATE_AVAIL: case VP_STATE_SYNC: + case VP_STATE_ASYNC_STARTING: case VP_STATE_REMOVED: return -EINVAL; case VP_STATE_ASYNC: