Re: [RFC PATCH 2/5] KVM: arm64: Add KVM_PGTABLE_PROT_DIRTY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Silent data corruption and loss of guest memory due to missing host folio dirty tracking with hardware Dirty Bit Management (HW DBM).
--

commit c5bf76c3d6bf3edf785c54c123328d74e2186585
Author: Leonardo Bras <leo.bras@arm.com>
KVM: arm64: Add KVM_PGTABLE_PROT_DIRTY

This commit introduces KVM_PGTABLE_PROT_DIRTY to distinguish between writable
but not dirty pages and writable and dirty pages in the Stage2 PTE descriptor.
It evaluates these concepts on a per-case basis across KVM ARM64 page tables.

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
[ ... ]
> @@ -731,8 +731,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p
>  	if (prot & KVM_PGTABLE_PROT_R)
>  		attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R;
>  
> -	if (prot & KVM_PGTABLE_PROT_W)
> -		attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM | KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> +	if (prot & KVM_PGTABLE_PROT_W) {
> +		attr |= KVM_PTE_LEAF_ATTR_HI_S2_DBM;
> +
> +		if (prot & KVM_PGTABLE_PROT_DIRTY)
> +			attr |= KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W;
> +	}

[Severity: Critical]
Could this translation enable hardware DBM bypass for pages not marked
PROT_DIRTY? This translates the software PROT_W without PROT_DIRTY into
the hardware PTE state (DBM=1, S2AP_W=0), setting up the HW DBM bypass.

[ ... ]
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -1681,10 +1683,10 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
>  	}
>  
>  out_unlock:
> -	kvm_release_faultin_page(kvm, page, !!ret, prot & KVM_PGTABLE_PROT_W);
> +	kvm_release_faultin_page(kvm, page, !!ret, prot & KVM_PGTABLE_PROT_DIRTY);

[Severity: Critical]
Does limiting the dirty tracking here to PROT_DIRTY instead of the mapping
writability (PROT_W) cause host folio dirty tracking to be missed when HAFDBS
is enabled?

[ ... ]
> @@ -1984,11 +1986,14 @@ static int kvm_s2_fault_compute_prot(const struct kvm_s2_fault_desc *s2fd,
>  
>  	*prot = KVM_PGTABLE_PROT_R;
>  
> -	if (s2vi->map_writable && (s2vi->device ||
> -				   !memslot_is_logging(s2fd->memslot) ||
> -				   kvm_is_write_fault(s2fd->vcpu)))
> +	if (s2vi->map_writable) {
>  		*prot |= KVM_PGTABLE_PROT_W;
>  
> +		if (s2vi->device || !memslot_is_logging(s2fd->memslot) ||
> +		    kvm_is_write_fault(s2fd->vcpu))
> +			*prot |= KVM_PGTABLE_PROT_DIRTY;
> +	}

[Severity: Critical]
On a read fault when HAFDBS is enabled, this leaves KVM_PGTABLE_PROT_DIRTY
unset while KVM_PGTABLE_PROT_W is set. Can this allow the hardware to
locklessly dirty the page later without KVM knowing?

[ ... ]
> @@ -2019,7 +2024,7 @@ static int kvm_s2_fault_map(const struct kvm_s2_fault_desc *s2fd,
>  			    void *memcache)
>  {
>  	enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_SHARED;
> -	bool writable = prot & KVM_PGTABLE_PROT_W;
> +	bool dirty = prot & KVM_PGTABLE_PROT_DIRTY;

[Severity: Critical]
By deriving the dirty status for host folio tracking strictly from PROT_DIRTY
instead of PROT_W, are we setting up the host MM to track the folio as clean
on read faults?

[ ... ]
> @@ -2082,7 +2087,7 @@ static int kvm_s2_fault_map(const struct kvm_s2_fault_desc *s2fd,
>  	}
>  
>  out_unlock:
> -	kvm_release_faultin_page(kvm, s2vi->page, !!ret, writable);
> +	kvm_release_faultin_page(kvm, s2vi->page, !!ret, dirty);

[Severity: Critical]
Releasing the folio to the host MM as clean on read faults could break host
folio dirty tracking.

If a guest later writes to this page and the CPU locklessly sets S2AP_W=1
without trapping, KVM won't be aware. Since KVM ARM64 does not harvest this
dirty bit during MMU notifier invalidations, the host Linux MM never learns
the folio was dirtied. When the host reclaims the folio under memory
pressure, does this result in data corruption and loss of guest memory by
discarding the modified data instead of writing it to swap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901171558.2674031-1-leo.bras@arm.com?part=2