Re: [PATCH RFC v2 03/50] mm: shmem: Implement guest_memfd provider operations for tmpfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: shmem: reference count leak on sbinfo->mpol in shmem_gmem_alloc_folio()
- [High] mm: shmem: shmem_gmem_alloc_folio() allocates unmovable folios as movable
--

commit 88b5d5c54fc1c479ad407c9b1ae1787c3dad600e
Author: Ackerley Tng <ackerleytng@google.com>

mm: shmem: Implement guest_memfd provider operations for tmpfs

This commit adds guest_memfd_provider_operations to file_operations and
implements tmpfs memory provider callbacks on shmem_dir_operations so that a
tmpfs root directory file descriptor can provide backing memory to
guest_memfd.

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 848316eaa7f4f..2a9faf8be7382 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c

[ ... ]

> @@ -5237,6 +5245,124 @@ static const struct inode_operations shmem_special_inode_operations = {
>  #endif
>  };
>  
> +#ifdef CONFIG_KVM_GUEST_MEMFD

[ ... ]

> +static struct folio *shmem_gmem_alloc_folio(void *provider, pgoff_t index,
> +					    struct mempolicy *mpol)
> +{
> +	struct mempolicy *sb_mpol = NULL;
> +	struct vfsmount *mnt = provider;
> +	struct shmem_sb_info *sbinfo;
> +	struct folio *folio;
> +
> +	sbinfo = SHMEM_SB(mnt->mnt_sb);
> +	if (sbinfo->max_blocks &&
> +	    !percpu_counter_limited_add(&sbinfo->used_blocks,
> +					sbinfo->max_blocks, 1))
> +		return ERR_PTR(-ENOSPC);
> +
> +	if (!mpol) {
> +		sb_mpol = shmem_get_sbmpol(sbinfo);
> +		mpol = sb_mpol;
> +	}
> +
> +	if (mpol)
> +		folio = folio_alloc_mpol(GFP_HIGHUSER_MOVABLE, 0, mpol, index,
> +					 numa_node_id());
> +	else
> +		folio = folio_alloc(GFP_HIGHUSER_MOVABLE, 0);

[Severity: High]
Should shmem_gmem_alloc_folio() allocate folios with GFP_HIGHUSER instead of
GFP_HIGHUSER_MOVABLE?

In virt/kvm/guest_memfd.c, kvm_gmem_get_folio() inserts the folio returned by
gmem_provider_alloc_folio() -> shmem_gmem_alloc_folio() into the guest_memfd
inode->i_mapping via filemap_add_folio(). However, kvm_gmem_init_inode()
configures that mapping as inaccessible and unevictable:

virt/kvm/guest_memfd.c:kvm_gmem_init_inode() {
	...
	mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);

	/*
	 * guest_memfd memory is neither migratable nor swappable: set
	 * inaccessible to gate off both.
	 */
	mapping_set_inaccessible(inode->i_mapping);
	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
	...
}

Because mapping_inaccessible() is true, move_to_new_folio() in mm/migrate.c
and isolate_migratepages_block() in mm/compaction.c reject migration with
-EOPNOTSUPP, kvm_gmem_migrate_folio() triggers WARN_ON_ONCE(1) and returns
-EINVAL, and shmem_gmem_attach() requires sbinfo->noswap. Allocating these
folios with __GFP_MOVABLE allows unmovable and unreclaimable guest_memfd
pages to be placed in ZONE_MOVABLE, MIGRATE_CMA, and MIGRATE_MOVABLE
pageblocks, which can cause CMA allocations, compaction, and memory
hot-unplug to fail.

> +
> +	mpol_cond_put(sb_mpol);

[Severity: High]
Does mpol_cond_put(sb_mpol) leak a reference on sbinfo->mpol when
shmem_gmem_alloc_folio() is called with a NULL mpol on a tmpfs mount
configured with an mpol= mount option?

When mpol is NULL, shmem_get_sbmpol() in mm/shmem.c unconditionally takes a
reference on sbinfo->mpol via mpol_get():

mm/shmem.c:shmem_get_sbmpol() {
	struct mempolicy *mpol = NULL;
	if (sbinfo->mpol) {
		raw_spin_lock(&sbinfo->stat_lock);
		mpol = sbinfo->mpol;
		mpol_get(mpol);
		raw_spin_unlock(&sbinfo->stat_lock);
	}
	return mpol;
}

However, mpol_cond_put() in include/linux/mempolicy.h only drops the
reference if MPOL_F_SHARED is set:

include/linux/mempolicy.h:mpol_cond_put() {
	if (mpol_needs_cond_ref(pol))
		__mpol_put(pol);
}

Since sbinfo->mpol is allocated via mpol_parse_str() -> mpol_new() without
MPOL_F_SHARED, mpol_cond_put(sb_mpol) is a no-op and leaks a reference on
every allocation, preventing sbinfo->mpol from being freed on remount or
unmount (and eventually wrapping atomic_t refcnt). Should this call
mpol_put(sb_mpol) instead, matching shmem_show_options() and
mpol_shared_policy_init()?

> +
> +	if (!folio) {
> +		if (sbinfo->max_blocks)
> +			percpu_counter_sub(&sbinfo->used_blocks, 1);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	return folio;
> +}

[ ... ]

>

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