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

[Leonardo Bras <leo.bras@arm.com>]

On Tue, Sep 01, 2026 at 05:34:52PM +0000, sashiko-bot@kernel.org wrote:
> 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.

That's the desired effect, as we can use HAFDBS/HDBSS to automatically 
update and track them.

> 
> [ ... ]
> > 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?
> 

Yes, that's why for dirty-tracking host should either disable HAFDBS or 
enable HDBSS if available.

> [ ... ]
> > @@ -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?

That's the desired behavior. If we need tracking, see above comment.

> 
> [ ... ]
> > @@ -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?
> 

Yes. The main idea is that a read fault does _not_ dirty a folio, just as 
it already happens when dirty-tracking is enabled.

> [ ... ]
> > @@ -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?

Ah, good catch. IIUC:
CPU0					CPU1
kvm_s2_fault_map - WC - pageA
	sets dirty=false
	...				HAFDBS sets pageA dirty
	kvm_release_faultin_page
		kvm_release_page_clean

So, in this case, I think it would be better to use writable instead of 
clean, as IIUC there should be no issue marking a clean page as dirty other 
than a bit of overhead on calling kvm_set_page_dirty().

Does it work?

Thanks!
Leo