drivers/hv/mshv_vtl: fix fabricated sidecar VP run completions

[John Starks (jstarks)]

sidecar_scan_vps() decides that an async run has completed when it observes cpu_status == CPU_STATUS_IDLE together with vp_state == VP_STATE_ASYNC. CPU_STATUS_IDLE is however equally true before a 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, and 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 is visible 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 both against the preceding cmpxchg() and against the cpu_status load that follows; a release store would leave that load free to be hoisted, losing the re-arm below. Invert the load order in the scan to match, so that observing VP_STATE_ASYNC implies observing CPU_STATUS_RUN.

The AP may 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.

Fixing the above required looking closely at the surrounding completion-reporting path, which turned up three further defects. They are fixed here as well, but none of them is needed to close the race above; the ASYNC_STARTING change alone is self-contained if a minimal backport is wanted.

First, split the locking out of the scan into the callers, renaming it sidecar_scan_vps_locked(). The claim loop in sidecar_scan_next_stopped() previously 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 whoever 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 observe 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() rather than having both callers re-read num_vps_stopped under the same lock. The early return has to report the count too: when needs_vp_scan is clear the scan does nothing, but entries marked by a previous scan may still be waiting to be claimed.

[Copilot]

Pull request overview

This PR fixes a race in the Hyper-V VTL sidecar async completion path where scans could fabricate a “run completion” by observing CPU_STATUS_IDLE before an async run was actually started, leading to userspace consuming stale command/intercept data.

Changes:

• Introduces VP_STATE_ASYNC_STARTING and publishes VP_STATE_ASYNC only after cpu_status is set to CPU_STATUS_RUN, using xchg() for ordering.
• Reworks VP scan logic into sidecar_scan_vps_locked() and moves locking responsibility to callers to prevent double-claim/double-decrement issues.
• Adjusts wakeups and scan accounting so pending completions aren’t lost and callers don’t re-read counters redundantly.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Steven Malis (smalis-msft)]

We now have test confirmation that this PR appears to fix the bug.

[Naman Jain (namancse)]

LGTM. Please port it to 6.18 main branch as well, once this is merged.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

drivers/hv/mshv_vtl_sidecar.c:226

• Same as the increment path: this decrement computes the new value using an unannotated plain load (dev->num_vps_stopped - 1) while other threads read the counter locklessly with READ_ONCE(). Use READ_ONCE() on the RHS to avoid KCSAN data-race reports / torn instrumentation when a reader is evaluating the wait predicate concurrently.

		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;

drivers/hv/mshv_vtl_sidecar.c:196

• num_vps_stopped is read locklessly in the wait predicate (via READ_ONCE() in sidecar_read()), but the increment here computes the new value using an unannotated plain load (dev->num_vps_stopped + 1). This can trigger KCSAN data-race reports and is inconsistent with the lockless read side. Prefer using READ_ONCE() for the RHS (or keeping it atomic) even though the update is under scan_mutex.

This issue also appears on line 224 of the same file.

		WRITE_ONCE(dev->vp_state[i], VP_STATE_ASYNC_STOPPED);
		WRITE_ONCE(dev->num_vps_stopped, dev->num_vps_stopped + 1);
		stopped = true;

[Steven Malis (smalis-msft)]

Ported in #157