Re: [PATCH v10 09/41] KVM: guest_memfd: Filter both shared and private when invalidating

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 8/20/2026 9:32 AM, Sean Christopherson wrote:
> On Mon, Aug 10, 2026, Ackerley Tng wrote:
>> Sean, do you know if looking up attributes in gmem to feed the KVM MMU
>> the smallest set of pages to zap will improve performance significantly?
>> Or if there's any other reason to do this lookup (more complexity in
>> gmem)?
> 
> While working through this with Ackerley, I realized this patch is buggy.  When
> in-place conversion is NOT supported, then as evidenced by the current code,
> invalidations are guaranteed to only affect one of SHARED vs. PRIVATE.  And if
> we change that to zap both, we risk overzapping.  I.e. it's not just the cost of
> the extra MMU walk, it could also be a functional bug.
> 
> Specifically, if KVM zaps both when SHARED vs. PRIVATE is tracked per-VM, then a
> PUNCH_HOLE operation on a PRIVATE guest_memfd will incorrectly zap SHARED mappings
> that have nothing to do with that gmem instance (because they're mapped via a VMA,
> not a gmem fd).  And vice versa, a PUNCH_HOLE on a SHARED gmem (if userspace is
> using an INIT_SHARED gmem for the shared branch of a memslot) could invalidate the
> PRIVATE mappings (of a different gmem instance).
> 
> That latter case in particular would be a functional bug, as spuriously zapping
> PRIVATE SPTEs is fatal to TDX (destroys the memory contents).
> 
> So, for the initial change, we want this (full patch below):
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 75979c885e03..8ff2ec148614 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -138,6 +138,9 @@ static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index)
>   
>   static enum kvm_gfn_range_filter kvm_gmem_get_invalidate_filter(struct inode *inode)
>   {
> +	if (gmem_in_place_conversion)
> +		return KVM_FILTER_SHARED | KVM_FILTER_PRIVATE;
> +
>   	if (GMEM_I(inode)->flags & GUEST_MEMFD_FLAG_INIT_SHARED)
>   		return KVM_FILTER_SHARED;
>   
> 
> And then in the main in-place conversion patch, have the conversion flow to only
> zap tap the "previous" types (with prep work as needed).  Ideally, that would be
> done *after* the main conversion patch, i.e. as an optimization, so that we get a
> nice bisection point if it's somehow buggy.  However, Ackerley pointed out that the
> conversion flow invalidates the entire range if the attributes of any gfn within
> the range is changing.  Addressing that would be rather annoying, e.g. there would
> need to be multiple invalidation ranges to deal with interpolated conversions,
> so going straight to a "zap only the previous" is probably the least awful option.
> 
> ---
> From: Sean Christopherson <seanjc@google.com>
> Date: Wed, 19 Aug 2026 18:06:38 -0700
> Subject: [PATCH] KVM: guest_memfd: Invalidate both SHARED and PRIVATE mappings
>   for in-place conversions
> 
> When removing one or more folios from a guest_memfd instance, invalidate
> both SHARED and PRIVATE mappings if in-place conversion is enabled, because
> stating the obvious, KVM needs to ensure that all mappings to the folio(s)
> are dropped.
> 
> Opportunistically rename the helper to capture that it returns the a filter
> for all gfns in anticipation of zapping only the previous mapping types on
> conversion.  I.e. when doing in-place conversion to PRIVATE, only SHARED
> mappings need to be zapped (ignoring that KVM would ideally not invalidate
> ranges whose attributes aren't changing in the first place).
> 
> Note, precisely zapping only the possible mapping types when in-place
> conversion is disabled is important for functional correctness, not just
> for performance.  Specifically, if KVM zaps both when SHARED vs. PRIVATE is
> tracked per-VM, then a PUNCH_HOLE operation on a PRIVATE guest_memfd will
> incorrectly zap SHARED mappings that have nothing to do with that gmem
> instance (because they're mapped via a VMA, not a gmem fd).  And vice versa,
> a PUNCH_HOLE on a SHARED gmem (if userspace is using an INIT_SHARED gmem
> for the shared branch of a memslot) could invalidate the PRIVATE mappings
> of a different gmem instance.  The latter case in particular would be a
> functional bug, as spuriously zapping PRIVATE SPTEs is fatal to TDX, as
> doing so destroys the contents of the memory.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   virt/kvm/guest_memfd.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 75979c885e03..7ae05ce3cd14 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -136,8 +136,11 @@ static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index)
>   	return folio;
>   }
>   
> -static enum kvm_gfn_range_filter kvm_gmem_get_invalidate_filter(struct inode *inode)
> +static enum kvm_gfn_range_filter kvm_gmem_get_all_gfns_filter(struct inode *inode)
>   {
> +	if (gmem_in_place_conversion)
> +		return KVM_FILTER_SHARED | KVM_FILTER_PRIVATE;

If I understand correctly, above diff is dead code and will change 
according to

   And then in the main in-place conversion patch, have the conversion
   flow to only zap tap the "previous" types (with prep work as needed).

If so, why bother adding the change?

>   	if (GMEM_I(inode)->flags & GUEST_MEMFD_FLAG_INIT_SHARED)
>   		return KVM_FILTER_SHARED;
>   
> @@ -188,11 +191,9 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>   static void kvm_gmem_invalidate_start(struct inode *inode, pgoff_t start,
>   				      pgoff_t end)
>   {
> -	enum kvm_gfn_range_filter attr_filter;
> +	enum kvm_gfn_range_filter attr_filter  = kvm_gmem_get_all_gfns_filter(inode);
>   	struct gmem_file *f;
>   
> -	attr_filter = kvm_gmem_get_invalidate_filter(inode);
> -
>   	kvm_gmem_for_each_file(f, inode)
>   		__kvm_gmem_invalidate_start(f, start, end, attr_filter);
>   }
> @@ -344,7 +345,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>   	 * memory, as its lifetime is associated with the inode, not the file.
>   	 */
>   	__kvm_gmem_invalidate_start(f, 0, -1ul,
> -				    kvm_gmem_get_invalidate_filter(inode));
> +				    kvm_gmem_get_all_gfns_filter(inode));
>   	__kvm_gmem_invalidate_end(f, 0, -1ul);
>   
>   	list_del(&f->entry);
> 
> base-commit: 620f8362aaec3848ee8f465949a0701175fe0896
> --