Re: [PATCH 21/24] KVM: x86/mmu: Obsolete all roots if memattr contains gPTEs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbounded iteration over a potentially massive guest-controlled GFN range while holding `mmu_lock` for write causes host soft lockups.
- [High] Invalidated TDP MMU roots are never zapped, leading to a massive permanent memory leak of page tables and root references.
- [Medium] A `trace_printk()` call was left in the production code path, creating an information leak and triggering a prominent boot warning.
--

--- Patch [21]: [PATCH 21/24] KVM: x86/mmu: Obsolete all roots if memattr contains gPTEs ---
commit 6d375c0ae74e01be130495175a78c166ea5a1aac
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: x86/mmu: Obsolete all roots if memattr contains gPTEs

This commit introduces a mechanism to zap obsolete pages and all mmu roots
when applying memory protection attributes against a GPTE page.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 8e85f672a08d5..676bc6b61f032 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c

[ ... ]

> @@ -8281,6 +8297,33 @@ bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
>  				hugepage_set_mixed(slot, gfn, level);
>  		}
>  	}
> +
> +	/*
> +	 * There are special considerations when applying an memory protection
> +	 * attibute against a GPTE page. If set read-only, access/dirty bits
> +	 * within that page shouldn't be updated. If set non-accesible,
> +	 * accessing a virtual address that requires traversing that GPTE page
> +	 * should fault.
> +	 *
> +	 * On TDP enabled guests, the CPU faults on the GPTE address upon
> +	 * detecting such a situation.
> +	 *
> +	 * On non-TDP, upon detecting this situation, and based on the fact it
> +	 * should be a rare occasion, invalidate all the mmu roots.
> +	 */
> +	for (gfn_t gfn = range->start; gfn < range->end; gfn++) {

[Severity: High]
Does this unbounded loop over a guest-controlled range cause host soft
lockups? It iterates over every 4K page from range->start to range->end
while kvm->mmu_lock is held for write, disabling preemption. If a massive
range is passed (e.g., hundreds of gigabytes), iterating millions of pages
without yielding could stall the host CPU.

> +		for_each_gfn_valid_sp_with_gptes(kvm, sp, gfn) {
> +			gen_update = true;
> +			trace_printk("needs gen update! %llx\n", gfn);

[Severity: Medium]
Is this trace_printk() intended for production? Using it unconditionally
allocates tracing ring buffers globally and emits a prominent warning to
dmesg at boot, which is generally avoided in upstream code.

> +			goto exit_loop;
> +		}
> +	}
> +exit_loop:
> +	if (gen_update)
> +		__kvm_mmu_zap_all_fast(kvm);
> +
> +	// todo: what to do with kvm_tdp_mmu_zap_invalidated_roots()?

[Severity: High]
Does this sequence leak KVM MMU page tables and root structures?

When tdp_mmu_enabled is true, __kvm_mmu_zap_all_fast() marks TDP roots as
invalid and takes a reference to them via kvm_tdp_mmu_invalidate_roots().

The caller must subsequently invoke kvm_tdp_mmu_zap_invalidated_roots()
to consume these scheduled roots and put the gifted references. Since this
cleanup step is skipped here, the roots appear to be leaked permanently.

> +	// add kvm_arch_post_set_memory_attributes_unlocked?
> +	return false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716181456.402786-1-pbonzini@redhat.com?part=21