Re: [PATCH v4 12/19] mm/shmem_luo: Retrieve tmpfs files into a file created by userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/shmem_luo: tail pages dropped on folio split in
  shmem_luo_retrieve_into()
- [Medium] mm/shmem_luo: race with khugepaged/MADV_COLLAPSE in
  shmem_luo_retrieve_into()
--

commit 780f293994f8e014ce055385564e7193d31d8556
Author: David Matlack <dmatlack@google.com>

mm/shmem_luo: Retrieve tmpfs files into a file created by userspace

This commit adds support for retrieving preserved tmpfs files into a
userspace-created file on a tmpfs mount via LIVEUPDATE_SESSION_RETRIEVE_FD,
and adds SHMEM_LUO_FLAG_TMPFS to distinguish tmpfs files from memfds.

> diff --git a/mm/shmem_luo.c b/mm/shmem_luo.c
> index bdf90aac5820f..7242664ee4aee 100644
> --- a/mm/shmem_luo.c
> +++ b/mm/shmem_luo.c
[ ... ]
> @@ -577,6 +603,86 @@ static int shmem_luo_restore_folios(struct file *file,
>  	return err;
>  }
>  
> +/*
> + * Restore a preserved tmpfs file into the file that userspace passed in the
> + * retrieve argument.
> + */
> +static int shmem_luo_retrieve_into(struct liveupdate_file_op_args *args,
> +				   struct shmem_luo_ser *ser)
> +{
> +	const struct liveupdate_shmem_retrieve_arg *arg = args->arg;
> +	struct inode *inode;
> +	struct file *file;
> +	int err;
> +
> +	if (!arg)
> +		return -EINVAL;
> +
> +	CLASS(fd, target)(arg->fd);
> +	if (fd_empty(target))
> +		return -EBADF;
> +
> +	file = fd_file(target);
> +	inode = file_inode(file);
> +
> +	/* Like the preserved file, the file must be on a tmpfs mount. */
> +	if (!shmem_file(file) || shmem_inode_on_internal_mount(inode))
> +		return -EINVAL;
> +
> +	if (!(file->f_mode & FMODE_WRITE))
> +		return -EBADF;
> +
> +	/* Fill the file like a write, which also takes these locks. */
> +	file_start_write(file);
> +	inode_lock(inode);
> +
> +	err = -EPERM;
> +	if (IS_IMMUTABLE(inode))
> +		goto unlock;
> +
> +	err = -EINVAL;
> +	if (i_size_read(inode) || !mapping_empty(inode->i_mapping))
> +		goto unlock;
> +
> +	err = liveupdate_claim_file(args, file);
> +	if (err)
> +		goto unlock;
> +
> +	err = file_modified(file);
> +	if (err)
> +		goto unclaim;
> +
> +	/* The preserved state is consumed from here on. */
> +	args->retryable = false;
> +
> +	/*
> +	 * The folios are past the end of the file until i_size is set below,
> +	 * so reads, page faults and memfd_pin_folios() don't see them, and
> +	 * they can be removed again if adding one fails.
> +	 */
> +	err = shmem_luo_restore_folios(file, ser);

[Severity: Medium]
Can khugepaged or MADV_COLLAPSE race with shmem_luo_restore_folios() if the
target tmpfs file is mmap()ed?

shmem_mmap_prepare() does not take inode_lock(inode), and
shmem_luo_retrieve_into() checks mapping_empty() but not mapping_mapped().
Unlike shmem_fallocate(), which leaves newly allocated folios beyond i_size as
!uptodate, shmem_luo_retrieve_folios() marks each restored folio uptodate
before unlocking it on the LRU:

CPU0                                  CPU1
shmem_luo_retrieve_into()
  inode_lock(inode)
  args->retryable = false
  shmem_luo_restore_folios()
    shmem_luo_retrieve_folios()
      /* restores folio at index 0 */
      shmem_add_to_page_cache()
      folio_mark_uptodate(folio)
      folio_unlock(folio)
                                      madvise(..., MADV_COLLAPSE)
                                        ...
                                          collapse_file()
                                            /* sees uptodate folio at 0,
                                             * treats indices 1..511 as
                                             * holes without checking
                                             * i_size, installs 2M folio */
      /* restores folio at index 1 */
      shmem_add_to_page_cache()
        --> returns -EEXIST           <-- permanent retrieve failure

Neither collapse_file() nor try_collapse_pte_mapped_thp() acquires
inode_lock(inode) or checks i_size_read(inode) when a shmem folio is already
uptodate:

mm/khugepaged.c:collapse_file() {
    ...
			if (xa_is_value(folio) || !folio_test_uptodate(folio)) {
				xas_unlock_irq(&xas);
				/* swap in or instantiate fallocated page */
				if (shmem_get_folio(mapping->host, index, 0,
						&folio, SGP_NOALLOC)) {
					result = SCAN_FAIL;
					goto xa_unlocked;
				}
				/* drain lru cache to help folio_isolate_lru() */
				lru_add_drain();
			} else if (folio_trylock(folio)) {
    ...
}

In addition to causing shmem_add_to_page_cache() to fail with -EEXIST after
args->retryable is set to false, try_collapse_pte_mapped_thp() can install a
huge PMD into userspace page tables while i_size is still 0, exposing folios
before the retrieve finishes.

> +	if (err) {
> +		shmem_truncate_range(inode, 0, (uoff_t)-1);
> +		goto unclaim;
> +	}
> +
> +	i_size_write(inode, ser->size);

[Severity: Medium]
Can a folio split during shmem_luo_restore_folios() drop the tail pages of a
restored large folio because both i_size and SHMEM_I(inode)->fallocend are 0?

shmem_luo_retrieve_folios() unlocks each restored folio and places it on the
LRU while subsequent iterations allocate XArray nodes and charge memcg and
quotas with GFP_HIGHUSER_MOVABLE and GFP_KERNEL. If page reclaim or migration
splits an already-inserted large folio before i_size_write() runs,
__folio_freeze_split_file() computes the EOF index from i_size and
shmem_fallocend():

mm/huge_memory.c:__folio_freeze_split_file() {
    ...
	end = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE);
	if (shmem_mapping(mapping))
		end = shmem_fallocend(mapping->host, end);
    ...
}

Because both i_size and info->fallocend are 0, end is 0, so every split
sub-folio with new_folio->index >= 1 is treated as beyond EOF, removed from
mapping->i_pages, and freed, silently corrupting the retrieved file. Setting
SHMEM_I(inode)->fallocend before shmem_luo_restore_folios() (as
shmem_fallocate() does) would prevent splits from discarding pages past
i_size.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111002358.1842967-1-dmatlack@google.com?part=12