Re: [PATCH RFC v2 04/50] KVM: guest_memfd: Support provider folio invalidation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: guest_memfd: AS_RELEASE_ALWAYS without release_folio() in kvm_gmem_aops
--

commit 761ef1ff98198357ffc1c8f5492a708cb9b2d9ab
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Support provider folio invalidation

This commit adds a provider invalidate_folio callback to guest_memfd and wires
it up to address_space_operations.invalidate_folio and the filemap_add_folio()
error path in kvm_gmem_get_folio(). It also sets AS_RELEASE_ALWAYS on the
inode mapping in kvm_gmem_init_inode() so truncate_cleanup_folio() invokes
.invalidate_folio.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 545c3e646e9c1..ee823f2ef0368 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -942,10 +950,24 @@ static void kvm_gmem_free_folio(struct folio *folio)
>  }
>  #endif
>  
> +static void kvm_gmem_invalidate_folio(struct folio *folio, size_t offset,
> +				      size_t len)
> +{
> +	struct inode *inode = folio->mapping->host;
> +	struct gmem_inode *gi = GMEM_I(inode);
> +
> +	/* guest_memfd only allows truncating full folios. */
> +	if (WARN_ON_ONCE(offset != 0 || len != folio_size(folio)))
> +		return;
> +
> +	gmem_provider_invalidate_folio(gi, folio);
> +}
> +
>  static const struct address_space_operations kvm_gmem_aops = {
>  	.dirty_folio = noop_dirty_folio,
>  	.migrate_folio	= kvm_gmem_migrate_folio,
>  	.error_remove_folio = kvm_gmem_error_folio,
> +	.invalidate_folio = kvm_gmem_invalidate_folio,
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
>  	.free_folio = kvm_gmem_free_folio,
>  #endif
> @@ -984,6 +1006,7 @@ static int kvm_gmem_init_inode(struct inode *inode, loff_t size, u64 flags)
>  	 */
>  	mapping_set_inaccessible(inode->i_mapping);
>  	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> +	mapping_set_release_always(inode->i_mapping);
>  
>  	gi->flags = flags;
>  

[Severity: High]
Does setting mapping_set_release_always() in kvm_gmem_init_inode() without
providing a .release_folio callback in kvm_gmem_aops cause filemap_release_folio()
to fall back to try_to_free_buffers() and allow mapping_evict_folio() to evict
guest_memfd folios?

When AS_RELEASE_ALWAYS is set, folio_needs_release(folio) returns true. If
mapping->a_ops->release_folio is NULL, filemap_release_folio() falls through
to try_to_free_buffers():

mm/filemap.c:filemap_release_folio() {
	if (!folio_needs_release(folio))
		return true;
	if (folio_test_writeback(folio))
		return false;

	if (mapping && mapping->a_ops->release_folio)
		return mapping->a_ops->release_folio(folio, gfp);
	return try_to_free_buffers(folio);
}

fs/buffer.c:try_to_free_buffers() {
	/* Misconfigured folio check */
	if (WARN_ON_ONCE(!folio_buffers(folio)))
		return true;
}

When a folio is allocated in kvm_gmem_get_folio(), filemap_add_folio() places
it on the unevictable LRU with PG_lru set. If the folio is not mapped into
userspace page tables (such as private guest_memfd folios or unmapped shared
folios, even while mapped in KVM Stage-2/TDP page tables), its refcount is 1
and it is clean.

If soft_offline_page() in mm/memory-failure.c runs on that PFN (or if
__do_fault() in mm/memory.c encounters a PageHWPoison guest_memfd folio), it
calls mapping_evict_folio() in mm/truncate.c:

soft_offline_page()
  -> get_hwpoison_page()
  -> soft_offline_in_use_page()
    -> mapping_evict_folio()
      -> filemap_release_folio()
        -> try_to_free_buffers()
      -> remove_mapping()

Because mapping_evict_folio() does not check mapping_unevictable(), it calls
filemap_release_folio(), triggers WARN_ON_ONCE(!folio_buffers(folio)) in
try_to_free_buffers(), and receives true. It then proceeds to remove_mapping(),
which removes the folio from inode->i_mapping without holding
filemap_invalidate_lock, without zapping KVM Stage-2 mappings, and without
invoking kvm_gmem_invalidate_folio(), leaking the provider's allocation
accounting (such as tmpfs sbinfo->used_blocks).

Should kvm_gmem_aops also implement .release_folio (or avoid
mapping_set_release_always()) so that filemap_release_folio() does not fall
through to try_to_free_buffers() and permit eviction via mapping_evict_folio()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-gmem-alternative-providers-v2-0-5dd1dc6c660a@google.com?part=4