Re: [PATCH v2 13/16] iommufd: Persist iommu hardware pagetables for live update

[Pranjal Shrivastava <praan@google.com>]

On Mon, Apr 27, 2026 at 05:56:30PM +0000, Samiullah Khawaja wrote:
> From: YiFei Zhu <zhuyifei@google.com>
> 
> Register iommufd with the LUO framework and implement the preserve and
> unpreserve ops to save marked HWPTs.
> 
> To make sure mappings do not change during preserved state, add a
> liveupdate_immutable flag to IOAS. When an HWPT is preserved, its IOAS
> is marked immutable and any map/unmap attempts will fail with -EBUSY.
> This is synchronized using the domains_rwsem to prevent races with
> concurrent mapping operations.
> 
> The preserve callback iterates over the marked HWPTs, verifies that the
> backing memory pages are preserved, and calls iommu_domain_preserve() to
> preserve the associated IOMMU domain.
> 
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>

[...]

> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 111f4d42e210..3c88aa115d08 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -98,6 +98,9 @@ struct io_pagetable {
>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>  	struct rb_root_cached reserved_itree;
>  	u8 disable_large_pages;
> +#ifdef CONFIG_IOMMU_LIVEUPDATE
> +	bool liveupdate_immutable;
> +#endif
>  	unsigned long iova_alignment;
>  };
>  
> @@ -379,7 +382,7 @@ struct iommufd_hwpt_paging {
>  	bool enforce_cache_coherency : 1;
>  	bool nest_parent : 1;
>  #ifdef CONFIG_IOMMU_LIVEUPDATE
> -	bool liveupdate_preserve : 1;
> +	bool liveupdate_preserved : 1;

Ahh okay, that's why I didn't find any reference earlier. (I searched
for liveupdate_preserve), Please ignore the remnant comment in Patch 12.

>  	u64 liveupdate_token;
>  #endif

[...]

> +
> +static int iommufd_preserve_hwpt(struct iommufd_hwpt_paging *hwpt,
> +				 struct iommufd_hwpt_ser *hwpt_ser,
> +				 struct liveupdate_session *session)
> +{
> +	struct iommu_domain_ser *domain_ser;
> +	bool ioas_made_immutable = false;
> +	int rc;
> +
> +	if (!hwpt->ioas->iopt.liveupdate_immutable) {
> +		/*
> +		 * Make IOAS immutable so the DMA mappings do not change while
> +		 * the HWPT is preserved. Since one IOAS can have multiple
> +		 * HWPTs, if an error occurs this call needs to make the IOAS
> +		 * mutable again if it was the one that made it immutable.
> +		 */
> +		ioas_made_immutable = true;
> +		ioas_set_immutable(hwpt->ioas, true);
> +
> +		rc = check_iopt_pages_preserved(session, hwpt);
> +		if (rc)
> +			goto err;
> +	}

Nit:
I'm thinking what happens for a shared IOAS situation? Say, 2 devices,
in the same container, behind 2 different IOMMUs, sharing the IOAS. Each
device will have it's own HWPT (N:1 mapping for HWPT v/s IOAS)

So, what happens if Device 1 succeeds with preservation but device 2
doesn't? For example:

1. Preserve Device 1:
 - It sees liveupdate_immutable is false.
 - Sets ioas_made_immutable = true on its local stack.
 - Flips IOAS to immutable.
 - Preservation succeeds.

2. Preserve Device 2:
 - It sees liveupdate_immutable is already true (because Device 1 set it).
 - Sets ioas_made_immutable = false on its local stack.
 - The Failure: iommu_domain_preserve fails for Device 2.
 - The Jump: Hits goto err;

Now, inside the err: label for device 2, it checks 
if (ioas_made_immutable), since it is FALSE for device 2,
it does nothing. 

I agree we return an error code to the caller which finally cleans it up
well, but I'm considering if we should make liveupdate_immutable
refcountable? Since the error handling in iommufd_preserve_hwpt() is
logically incomplete for shared IOAS as it only attempts to restore 
mutability if the current HWPT set immutable = true;

> +
> +	hwpt_ser->token = hwpt->liveupdate_token;
> +	hwpt_ser->reclaimed = false;
> +
> +	rc = iommu_domain_preserve(hwpt->common.domain, &domain_ser);
> +	if (rc < 0)
> +		goto err;
> +
> +	hwpt_ser->domain_data = virt_to_phys(domain_ser);
> +	return 0;
> +
> +err:
> +	if (ioas_made_immutable)
> +		ioas_set_immutable(hwpt->ioas, false);
> +
> +	return rc;
> +}

[...]

> +
> +static int iommufd_liveupdate_preserve(struct liveupdate_file_op_args *args)
> +{
> +	struct iommufd_ctx *ictx = iommufd_ctx_from_file(args->file);
> +	struct iommufd_hwpt_paging *hwpt;
> +	struct iommufd_ser *iommufd_ser;
> +	struct iommufd_object *obj;
> +	unsigned int nr_hwpts;
> +	unsigned long index;
> +	unsigned int i;
> +	void *mem;
> +	int rc;
> +
> +	if (IS_ERR(ictx))
> +		return PTR_ERR(ictx);
> +
> +	mutex_lock(&ictx->liveupdate_mutex);
> +
> +	/* Count the number of HWPTs to preserve */
> +	nr_hwpts = 0;
> +	xa_lock(&ictx->objects);
> +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
> +		if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
> +			continue;
> +
> +		hwpt = to_hwpt_paging(container_of(obj, struct iommufd_hw_pagetable, obj));
> +		if (!hwpt->common.domain) {
> +			rc = -EINVAL;
> +			xa_unlock(&ictx->objects);
> +			goto out_unlock;
> +		}
> +		nr_hwpts++;
> +	}
> +	xa_unlock(&ictx->objects);
> +
> +	mem = kho_alloc_preserve(struct_size(iommufd_ser,
> +					     hwpt_array, nr_hwpts));
> +	if (!mem) {
> +		rc = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	iommufd_ser = mem;
> +	iommufd_ser->nr_hwpts = nr_hwpts;

Nit: Can there be a TOCTOU here? We first count nr_hwpts in the first
loop, but actually preserve them in the loop below. Is it possible for a
Guest to race with these loops and destroy a HWPT? 

That could cause a bug in the new kernel as it may try to restore
nr_hwpts which is one more than the preserved HWPTs.

> +
> +	/* Preserve HWPTs */
> +	i = 0;
> +	xa_lock(&ictx->objects);
> +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
> +		if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
> +			continue;
> +
> +		if (!iommufd_lock_obj(obj)) {
> +			rc = -ENOENT;
> +			xa_unlock(&ictx->objects);
> +			goto out_unpreserve;
> +		}
> +
> +		/*
> +		 * HWPT is locked so it will not be destroyed. The xarray lock
> +		 * can be released here before preserving the HWPT.
> +		 */
> +		xa_unlock(&ictx->objects);
> +		hwpt = to_hwpt_paging(container_of(obj, struct iommufd_hw_pagetable, obj));
> +		rc = iommufd_preserve_hwpt(hwpt, &iommufd_ser->hwpt_array[i++], args->session);
> +		if (rc) {
> +			iommufd_put_object(ictx, obj);
> +			goto out_unpreserve;
> +		}
> +
> +		/* Mark as preserved */
> +		hwpt->liveupdate_preserved = true;
> +		xa_lock(&ictx->objects);
> +	}
> +	xa_unlock(&ictx->objects);

[...]

> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index 9bdb2945afe1..3b0c0acb8856 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -55,6 +55,7 @@
>  #include <linux/overflow.h>
>  #include <linux/slab.h>
>  #include <linux/sched/mm.h>
> +#include <linux/memfd.h>
>  #include <linux/vfio_pci_core.h>
>  
>  #include "double_span.h"
> @@ -1421,6 +1422,7 @@ struct iopt_pages *iopt_alloc_file_pages(struct file *file,
>  
>  {
>  	struct iopt_pages *pages;
> +	int seals;
>  
>  	pages = iopt_alloc_pages(start_byte, length, writable);
>  	if (IS_ERR(pages))
> @@ -1428,6 +1430,11 @@ struct iopt_pages *iopt_alloc_file_pages(struct file *file,
>  	pages->file = get_file(file);
>  	pages->start = start - start_byte;
>  	pages->type = IOPT_ADDRESS_FILE;
> +
> +	seals = memfd_get_seals(file);
> +	if (seals > 0)
> +		pages->seals = seals;
> +

Can caching memfd seals create a TOCTOU issue?
IIUC, iopt_alloc_file_pages happens at map time, However, the userspace
is allowed to map a memfd and then apply the F_ADD_SEALS via fcntl() 
later in its setup sequence? For example a sequence like:

1. VMM creates a memfd. It has 0 seals.
2. VMM calls IOMMU_IOAS_MAP_FILE. IOMMUFD caches pages->seals = 0.
3. VMM finishes its setup and calls:
   fcntl(fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL).

4.VMM initiates Live Update.
5.check_iopt_pages_preserved looks at the cached pages->seals 
  (which is still 0), sees the seals are missing, & kills the LiveUpdate
  with -EINVAL, even though the file is properly sealed..

Thus, I guess we should dynamically check seals via memfd_get_seals() 

>  	return pages;
>  }
>  

Thanks,
Praan