Re: [PATCH v4 09/19] mm/shmem_luo: Leave freeing the preserved state to finish()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm/shmem_luo: preserved folios freed before finish() if
  retrieve_folios() fails
--

commit 07031901f696f6bb10658c21e386ae0237da0633
Author: David Matlack <dmatlack@google.com>

mm/shmem_luo: Leave freeing the preserved state to finish()

This commit defers freeing the serialized shmem state from
shmem_luo_retrieve() to shmem_luo_finish() so that preserved folios are not
leaked or freed prematurely when retrieve fails.

> diff --git a/mm/shmem_luo.c b/mm/shmem_luo.c
> index e747a04422954..b48d7f460493d 100644
> --- a/mm/shmem_luo.c
> +++ b/mm/shmem_luo.c
> @@ -390,13 +390,10 @@ static void shmem_luo_finish(struct liveupdate_file_op_args *args)
>  	struct shmem_luo_ser *ser;
>  
>  	/*
> -	 * If retrieve was successful, nothing to do. If it failed, retrieve()
> -	 * already cleaned up everything it could. So nothing to do there
> -	 * either. Only need to clean up when retrieve was not called.
> +	 * Preserved folios may be in use until the session is finished, so
> +	 * retrieve() leaves freeing the preserved state to finish(), whether it
> +	 * succeeds or not.
>  	 */
> -	if (args->retrieve_status)
> -		return;
> -
>  	ser = phys_to_virt(args->serialized_data);
>  	if (!ser)
>  		return;
> @@ -515,9 +512,34 @@ static int shmem_luo_retrieve_folios(struct file *file,
>  	return err;
>  }
>  
> -static int shmem_luo_retrieve(struct liveupdate_file_op_args *args)
> +/*
> + * Restore the preserved folios into @file. This consumes them even if it fails,
> + * since they are then either in @file or freed, so make sure finish() doesn't
> + * free them again.
> + */
> +static int shmem_luo_restore_folios(struct file *file,
> +				    struct shmem_luo_ser *ser)
>  {
>  	struct shmem_luo_folio_ser *folios_ser;
> +	u64 nr_folios = ser->nr_folios;
> +	int err;
> +
> +	if (!nr_folios)
> +		return 0;
> +
> +	ser->nr_folios = 0;
> +
> +	folios_ser = kho_restore_vmalloc(&ser->folios);
> +	if (!folios_ser)
> +		return -EINVAL;
> +
> +	err = shmem_luo_retrieve_folios(file, folios_ser, nr_folios);
> +	vfree(folios_ser);
> +	return err;
> +}

[ ... ]

> @@ -552,28 +571,15 @@ static int shmem_luo_retrieve(struct liveupdate_file_op_args *args)
>  	vfs_setpos(file, ser->pos, MAX_LFS_FILESIZE);
>  	i_size_write(file_inode(file), ser->size);
>  
> -	if (ser->nr_folios) {
> -		folios_ser = kho_restore_vmalloc(&ser->folios);
> -		if (!folios_ser) {
> -			err = -EINVAL;
> -			goto put_file;
> -		}
> -
> -		err = shmem_luo_retrieve_folios(file, folios_ser, ser->nr_folios);
> -		vfree(folios_ser);
> -		if (err)
> -			goto put_file;
> -	}
> +	err = shmem_luo_restore_folios(file, ser);
> +	if (err)
> +		goto put_file;
>  
>  	args->file = file;
> -	kho_restore_free(ser);
> -
>  	return 0;
>  
>  put_file:
>  	fput(file);
> -free_ser:
> -	kho_restore_free(ser);
>  	return err;
>  }

[Severity: Medium]
Does shmem_luo_retrieve() still free all preserved folios before
shmem_luo_finish() if shmem_luo_retrieve_folios() fails?

When shmem_luo_retrieve_folios() fails mid-loop (for example, if
mem_cgroup_charge() fails), its error path restores and puts the remaining
folios, and the jump to put_file in shmem_luo_retrieve() calls fput(file),
which evicts the inode and frees the folios already added to the page cache:

mm/shmem_luo.c:shmem_luo_retrieve_folios() {
	...
unlock_folio:
	folio_unlock(folio);
	folio_put(folio);
put_folios:
	...
	for (long j = i + 1; j < nr_folios; j++) {
		...
		folio = kho_restore_folio(phys);
		if (folio)
			folio_put(folio);
	}
}

If a preserved device still holds DMA mappings to those folios until the
session finishes, freeing them during retrieve() allows the pages to be
reallocated while still in use.

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