Re: [PATCH 2/2] mm/memfd_luo: keep memfd frozen until finish

[Pasha Tatashin]

> After a memfd is retrieved, it immediately goes back into "normal"
> operation. The file comes back unfrozen, and its pages can migrate, it
> can be truncated, and so on. If the memfd is depended on by something,
> like iommufd, that can open a window for folios to move around before
> the consumer can re-establish its references.
> 
> Finish is the conceptual point when a retrieved file can start to behave
> as "normal". So take an extra reference on the folios and freeze the
> memfd on retrieve until finish.
> 
> Suggested-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> Signed-off-by: Pratyush Yadav (Google) <pratyush@xxxxxxxxxx>
> Link: https://patch.msgid.link/20261103181916.2462825-3-pratyush@xxxxxxxxxx
>
> diff --git a/mm/memfd_luo.c b/mm/memfd_luo.c
> index 6b68220275eb..5abb9c0956af 100644
> --- a/mm/memfd_luo.c
> +++ b/mm/memfd_luo.c
> @@ -55,6 +55,14 @@
>   *   Only seals known to this LUO version (see ``MEMFD_LUO_ALL_SEALS``) may
>   *   be present; preservation fails with ``-EOPNOTSUPP`` otherwise.
>   *
> + * Restrictions
> + * ============
> + *
> + * While a memfd is preserved, and after it is retrieved until the session is
> + * finished, its size can't be changed and holes can't be punched in it.
> + * Operations that would do so (``ftruncate()``, ``fallocate()``, writes past
> + * the end of the file) fail.
> + *
>   * Non-Preserved Properties
>   * ========================
>   *
> @@ -383,33 +391,56 @@ static void memfd_luo_discard_folios(const struct memfd_luo_folio_ser *folios_se
>  	}
>  }
>  
> +/* Drop the references retrieve() holds on the folios added to the file. */
> +static void
> +memfd_luo_put_retrieved_folios(const struct memfd_luo_folio_ser *folios_ser,
> +			       u64 nr_folios)
> +{
> +	for (u64 i = 0; i < nr_folios; i++) {
> +		const struct memfd_luo_folio_ser *pfolio = &folios_ser[i];
> +
> +		if (pfolio->pfn)
> +			folio_put(pfn_folio(pfolio->pfn));
> +	}
> +}
> +
>  static void memfd_luo_finish(struct liveupdate_file_op_args *args)
>  {
>  	struct memfd_luo_folio_ser *folios_ser;
>  	struct memfd_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.
> -	 */
> -	if (args->retrieve_status)
> +	/* If retrieve failed, it already cleaned up everything it could. */
> +	if (args->retrieve_status < 0)
>  		return;
>  
>  	ser = phys_to_virt(args->serialized_data);
>  	if (!ser)
>  		return;
>  
> -	if (ser->nr_folios) {
> -		folios_ser = kho_restore_vmalloc(&ser->folios);
> -		if (!folios_ser)
> -			goto out;
> +	if (args->retrieve_status > 0) {
> +		/*
> +		 * Retrieve was successful, release the folios and unfreeze the
> +		 * file so it can be used normally.
> +		 */
> +		struct inode *inode = file_inode(args->file);
> +
> +		folios_ser = args->private_data;
> +
> +		inode_lock(inode);
> +		memfd_luo_put_retrieved_folios(folios_ser, ser->nr_folios);
> +		shmem_freeze(inode, false);
> +		inode_unlock(inode);
>  
> -		memfd_luo_discard_folios(folios_ser, ser->nr_folios);
>  		vfree(folios_ser);
> +	} else if (args->retrieve_status == 0 && ser->nr_folios) {
> +		/* Retrieve was not called, discard the preserved folios. */
> +		folios_ser = kho_restore_vmalloc(&ser->folios);
> +		if (folios_ser) {
> +			memfd_luo_discard_folios(folios_ser, ser->nr_folios);
> +			vfree(folios_ser);
> +		}

Reviewed-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> 

Side note (pre-existing, not an issue with this patch):

Initially, I wondered whether we should disallow auto-finishing an 
unretrieved memfd, in case a preserved device still had this memory 
mapped for DMA. However, if we reach this point, can_finish() has 
already passed for every other FD in this session, which means any 
possibly dependent FDs (e.g. iommufd/vfio) have already determined that 
this FD was not needed to be retrieved for their states.

-- 
Pasha