Re: [PATCH v3 09/15] KVM: x86/tdp_mmu: Centrally propagate to-present/atomic zap updates to external PTEs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Mutually exclusive `else if` logic in `__handle_changed_spte()` prevents external page table updates for parent SPTEs, and incorrect ordering creates a use-after-free hazard on error rollback.
- [High] Propagating deterministic errors from `set_external_spte()` causes an infinite retry loop in callers of `tdp_mmu_set_spte_atomic()`.
--

commit e8401f58d2841529854a98f404633950aa40f76c
Author: Sean Christopherson <seanjc@google.com>

KVM: x86/tdp_mmu: Centrally propagate to-present/atomic zap updates to external PTEs

This commit moves the propagation of to-present changes and atomic zap
changes to external PTEs from __tdp_mmu_set_spte_atomic() to
__handle_changed_spte(), centralizing SPTE change handling.

Link: https://lore.kernel.org/lkml/aYYn0nf2cayYu8e7@google.com [1]
Link: https://lore.kernel.org/kvm/aY_Dq3riRLUNFZfr@google.com [2]

> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c

[ ... ]

> @@ -578,29 +576,51 @@ static void handle_changed_spte(struct kvm *kvm, struct kvm_mmu_page *sp,

[ ... ]

>  	/*
>  	 * Recursively handle child PTs if the change removed a subtree from
>  	 * the paging structure.  Note the WARN on the PFN changing without the
>  	 * SPTE being converted to a hugepage (leaf) or being zapped.  Shadow
>  	 * pages are kernel allocations and should never be migrated.
> +	 *
> +	 * For the mirror page table, propagate changes to present or changes of
> +	 * leaf SPTEs to !present under shared mmu_lock to the external SPTE via
> +	 * set_external_spte() op.
>  	 */
>  	if (was_present && !was_leaf &&
> -	    (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> +	    (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed))) {
>  		handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> +	} else if (is_mirror_sp(sp) && (is_present || shared)) {
> +		int r;
> +
> +		r = kvm_x86_call(set_external_spte)(kvm, gfn, old_spte, new_spte, level);
> +		if (r)
> +			return r;
> +	}

[Severity: Critical]
Does this mutually exclusive else if logic skip external page table updates
for parent SPTEs? 

When replacing a non-leaf mirror SPTE with a leaf SPTE (such as during a
hugepage collapse) or performing a shared atomic zap on a non-leaf SPTE, the
first condition is met and handle_removed_pt() is called. Because of the
else if, the set_external_spte() call is skipped entirely, leaving the
hardware external page table desynchronized.

Additionally, if this is changed to an independent if block, could the order
of operations create a use-after-free on error rollback?

If handle_removed_pt() executes first, it irreversibly unlinks and schedules
child page tables for RCU freeing. If set_external_spte() then runs and
fails (e.g. TDX rejects it), __handle_changed_spte() returns an error. The
caller tdp_mmu_set_spte_atomic() will attempt to roll back by restoring the
parent SPTE to its old value. However, since the child page tables were
already freed, the restored SPTE would point to RCU-freed memory.

Should set_external_spte() be called before handle_removed_pt() to allow a
safe rollback?

> @@ -673,14 +668,51 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,

[ ... ]

> +	ret = __handle_changed_spte(kvm, sp, iter->gfn, iter->old_spte,
> +				    new_spte, iter->level, true);
> +	/*
> +	 * Unfreeze the mirror SPTE.  If updating the external SPTE failed,
> +	 * restore the old value so that the mirror SPTE isn't frozen in
> +	 * perpetuity, otherwise set the mirror SPTE to the new desired value.
> +	 */
> +	if (is_mirror_sptep(iter->sptep)) {
> +		if (ret)
> +			__kvm_tdp_mmu_write_spte(iter->sptep, iter->old_spte);
> +		else
> +			__kvm_tdp_mmu_write_spte(iter->sptep, new_spte);
> +	} else {
> +		/*
> +		 * Bug the VM if handling the change failed, as failure is only
> +		 * allowed if KVM couldn't update the external SPTE.
> +		 */
> +		KVM_BUG_ON(ret, kvm);
> +	}
> +	return ret;
>  }

[Severity: High]
Will propagating deterministic error codes from set_external_spte() cause
infinite retry loops in callers of tdp_mmu_set_spte_atomic()?

Callers of tdp_mmu_set_spte_atomic() generally assume that any non-zero
return value is a transient cmpxchg race (which typically returns -EBUSY),
and they implement an unconditional retry pattern on failure.

If set_external_spte() rejects an operation and returns a deterministic
error like -EINVAL (e.g. rejecting an atomic zap as mentioned in the commit
message) or -ENOMEM, this error is propagated back. Because callers do not
check the specific error code before retrying, won't this result in a
kernel livelock where the caller infinitely loops attempting the same doomed
operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528080856.10141-1-yan.y.zhao@intel.com?part=9