Re: [PATCH 11/14] iommufd-lu: Persist iommu hardware pagetables for live update

Samiullah Khawaja
2026-03-25T20:32:36Z

On Wed, Mar 25, 2026 at 08:08:48PM +0000, Pranjal Shrivastava wrote:
>On Tue, Feb 03, 2026 at 10:09:45PM +0000, Samiullah Khawaja wrote:
>> From: YiFei Zhu <zhuyifei@google.com>
>>
>> The caller is expected to mark each HWPT to be preserved with an ioctl
>> call, with a token that will be used in restore. At preserve time, each
>> HWPT's domain is then called with iommu_domain_preserve to preserve the
>> iommu domain.
>>
>> The HWPTs containing dma mappings backed by unpreserved memory should
>> not be preserved. During preservation check if the mappings contained in
>> the HWPT being preserved are only file based and all the files are
>> preserved.
>>
>> The memfd file preservation check is not enough when preserving iommufd.
>> The memfd might have shrunk between the mapping and memfd preservation.
>> This means that if it shrunk some pages that are right now pinned due to
>> iommu mappings are not preserved with the memfd. Only allow iommufd
>> preservation when all the iopt_pages are file backed and the memory file
>> was seal sealed during mapping. This guarantees that all the pages that
>> were backing memfd when it was mapped are preserved.
>>
>> Once HWPT is preserved the iopt associated with the HWPT is made
>> immutable. Since the map and unmap ioctls operates directly on iopt,
>> which contains an array of domains, while each hwpt contains only one
>> domain. The logic then becomes that mapping and unmapping is prohibited
>> if any of the domains in an iopt belongs to a preserved hwpt. However,
>> tracing to the hwpt through the domain is a lot more tedious than
>> tracing through the ioas, so if an hwpt is preserved, hwpt->ioas->iopt
>> is made immutable.
>>
>> When undoing this (making the iopts mutable again), there's never
>> a need to make some iopts mutable and some kept immutable, since
>> the undo only happen on unpreserve and error path of preserve.
>> Simply iterate all the ioas and clear the immutability flag on all
>> their iopts.
>>
>> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>> ---
>> drivers/iommu/iommufd/io_pagetable.c | 17 ++
>> drivers/iommu/iommufd/io_pagetable.h | 1 +
>> drivers/iommu/iommufd/iommufd_private.h | 25 ++
>> drivers/iommu/iommufd/liveupdate.c | 300 ++++++++++++++++++++++++
>> drivers/iommu/iommufd/main.c | 14 +-
>> drivers/iommu/iommufd/pages.c | 8 +
>> include/linux/kho/abi/iommufd.h | 39 +++
>> 7 files changed, 403 insertions(+), 1 deletion(-)
>> create mode 100644 include/linux/kho/abi/iommufd.h
>>
>> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
>> index 436992331111..43e8a2443793 100644
>> --- a/drivers/iommu/iommufd/io_pagetable.c
>> +++ b/drivers/iommu/iommufd/io_pagetable.c
>> @@ -270,6 +270,11 @@ static int iopt_alloc_area_pages(struct io_pagetable *iopt,
>> }
>>
>> down_write(&iopt->iova_rwsem);
>> + if (iopt_lu_map_immutable(iopt)) {
>> + rc = -EBUSY;
>> + goto out_unlock;
>> + }
>> +
>> if ((length & (iopt->iova_alignment - 1)) || !length) {
>> rc = -EINVAL;
>> goto out_unlock;
>> @@ -328,6 +333,7 @@ static void iopt_abort_area(struct iopt_area *area)
>> WARN_ON(area->pages);
>> if (area->iopt) {
>> down_write(&area->iopt->iova_rwsem);
>> + WARN_ON(iopt_lu_map_immutable(area->iopt));
>> interval_tree_remove(&area->node, &area->iopt->area_itree);
>> up_write(&area->iopt->iova_rwsem);
>> }
>> @@ -755,6 +761,12 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
>> again:
>> down_read(&iopt->domains_rwsem);
>> down_write(&iopt->iova_rwsem);
>> +
>> + if (iopt_lu_map_immutable(iopt)) {
>> + rc = -EBUSY;
>> + goto out_unlock_iova;
>> + }
>> +
>> while ((area = iopt_area_iter_first(iopt, start, last))) {
>> unsigned long area_last = iopt_area_last_iova(area);
>> unsigned long area_first = iopt_area_iova(area);
>> @@ -1398,6 +1410,11 @@ int iopt_cut_iova(struct io_pagetable *iopt, unsigned long *iovas,
>> int i;
>>
>> down_write(&iopt->iova_rwsem);
>> + if (iopt_lu_map_immutable(iopt)) {
>> + up_write(&iopt->iova_rwsem);
>> + return -EBUSY;
>> + }
>> +
>> for (i = 0; i < num_iovas; i++) {
>> struct iopt_area *area;
>>
>> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
>> index 14cd052fd320..b64cb4cf300c 100644
>> --- a/drivers/iommu/iommufd/io_pagetable.h
>> +++ b/drivers/iommu/iommufd/io_pagetable.h
>> @@ -234,6 +234,7 @@ struct iopt_pages {
>> struct { /* IOPT_ADDRESS_FILE */
>> struct file *file;
>> unsigned long start;
>> + u32 seals;
>> };
>> /* IOPT_ADDRESS_DMABUF */
>> struct iopt_pages_dmabuf dmabuf;
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index 6424e7cea5b2..f8366a23999f 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -94,6 +94,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 lu_map_immutable;
>> +#endif
>> unsigned long iova_alignment;
>> };
>>
>> @@ -712,12 +715,34 @@ iommufd_get_vdevice(struct iommufd_ctx *ictx, u32 id)
>> }
>>
>> #ifdef CONFIG_IOMMU_LIVEUPDATE
>> +int iommufd_liveupdate_register_lufs(void);
>> +int iommufd_liveupdate_unregister_lufs(void);
>> +
>> int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd);
>> +static inline bool iopt_lu_map_immutable(const struct io_pagetable *iopt)
>> +{
>> + return iopt->lu_map_immutable;
>> +}
>> #else
>> +static inline int iommufd_liveupdate_register_lufs(void)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline int iommufd_liveupdate_unregister_lufs(void)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd)
>> {
>> return -ENOTTY;
>> }
>> +
>> +static inline bool iopt_lu_map_immutable(const struct io_pagetable *iopt)
>> +{
>> + return false;
>> +}
>> #endif
>>
>> #ifdef CONFIG_IOMMUFD_TEST
>> diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c
>> index ae74f5b54735..ec11ae345fe7 100644
>> --- a/drivers/iommu/iommufd/liveupdate.c
>> +++ b/drivers/iommu/iommufd/liveupdate.c
>> @@ -4,9 +4,15 @@
>>
>> #include <linux/file.h>
>> #include <linux/iommufd.h>
>> +#include <linux/kexec_handover.h>
>> +#include <linux/kho/abi/iommufd.h>
>> #include <linux/liveupdate.h>
>> +#include <linux/iommu-lu.h>
>> +#include <linux/mm.h>
>> +#include <linux/pci.h>
>>
>> #include "iommufd_private.h"
>> +#include "io_pagetable.h"
>>
>> int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd)
>> {
>> @@ -47,3 +53,297 @@ int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd)
>> return rc;
>> }
>>
>> +static void iommufd_set_ioas_mutable(struct iommufd_ctx *ictx)
>> +{
>> + struct iommufd_object *obj;
>> + struct iommufd_ioas *ioas;
>> + unsigned long index;
>> +
>> + xa_lock(&ictx->objects);
>> + xa_for_each(&ictx->objects, index, obj) {
>> + if (obj->type != IOMMUFD_OBJ_IOAS)
>> + continue;
>> +
>> + ioas = container_of(obj, struct iommufd_ioas, obj);
>> +
>> + /*
>> + * Not taking any IOAS lock here. All writers take LUO
>> + * session mutex, and this writer racing with readers is not
>> + * really a problem.
>> + */
>> + WRITE_ONCE(ioas->iopt.lu_map_immutable, false);
>> + }
>> + xa_unlock(&ictx->objects);
>> +}
>> +
>> +static int check_iopt_pages_preserved(struct liveupdate_session *s,
>> + struct iommufd_hwpt_paging *hwpt)
>> +{
>> + u32 req_seals = F_SEAL_SEAL | F_SEAL_GROW | F_SEAL_SHRINK;
>> + struct iopt_area *area;
>> + int ret;
>> +
>> + for (area = iopt_area_iter_first(&hwpt->ioas->iopt, 0, ULONG_MAX); area;
>> + area = iopt_area_iter_next(area, 0, ULONG_MAX)) {
>> + struct iopt_pages *pages = area->pages;
>> +
>> + /* Only allow file based mapping */
>> + if (pages->type != IOPT_ADDRESS_FILE)
>> + return -EINVAL;
>
>That's an important check! should we document it somewhere for the user
>to know about it too? Perhaps in the uAPI header for the preserve IOCTL,
>so VMM authors are aware of the anonymous memory restriction upfront?

Agreed. Will add this in next revision.
>
>> +
>> + /*
>> + * When this memory file was mapped it should be sealed and seal
>> + * should be sealed. This means that since mapping was done the
>> + * memory file was not grown or shrink and the pages being used
>> + * until now remain pinnned and preserved.
>> + */
>> + if ((pages->seals & req_seals) != req_seals)
>> + return -EINVAL;
>> +
>> + /* Make sure that the file was preserved. */
>> + ret = liveupdate_get_token_outgoing(s, pages->file, NULL);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int iommufd_save_hwpts(struct iommufd_ctx *ictx,
>> + struct iommufd_lu *iommufd_lu,
>> + struct liveupdate_session *session)
>> +{
>> + struct iommufd_hwpt_paging *hwpt, **hwpts = NULL;
>> + struct iommu_domain_ser *domain_ser;
>> + struct iommufd_hwpt_lu *hwpt_lu;
>> + struct iommufd_object *obj;
>> + unsigned int nr_hwpts = 0;
>> + unsigned long index;
>> + unsigned int i;
>> + int rc = 0;
>> +
>> + if (iommufd_lu) {
>> + hwpts = kcalloc(iommufd_lu->nr_hwpts, sizeof(*hwpts),
>> + GFP_KERNEL);
>> + if (!hwpts)
>> + return -ENOMEM;
>> + }
>> +
>> + xa_lock(&ictx->objects);
>> + xa_for_each(&ictx->objects, index, obj) {
>> + if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
>> + continue;
>> +
>> + hwpt = container_of(obj, struct iommufd_hwpt_paging, common.obj);
>> + if (!hwpt->lu_preserve)
>> + continue;
>> +
>> + if (hwpt->ioas) {
>> + /*
>> + * Obtain exclusive access to the IOAS and IOPT while we
>> + * set immutability
>> + */
>> + mutex_lock(&hwpt->ioas->mutex);
>
>Doubling down on Ankit's comment here, we should absolutely avoid taking
>sleepable locks in atomic context. I can attempt testing the next series
>with lockdep enabled.
>
>> + down_write(&hwpt->ioas->iopt.domains_rwsem);
>> + down_write(&hwpt->ioas->iopt.iova_rwsem);
>> +
>> + hwpt->ioas->iopt.lu_map_immutable = true;
>> +
>> + up_write(&hwpt->ioas->iopt.iova_rwsem);
>> + up_write(&hwpt->ioas->iopt.domains_rwsem);
>> + mutex_unlock(&hwpt->ioas->mutex);
>
>Additionally, I'm wondering if this could be a helper function?
>iommufd_ioas_set_immutable?

I will have to restructure this part of the code to not take the mutex
in the atomic context. I will move it to a helper function in next
revision if there is opportunity to do that.
>
>> + }
>> +
>> + if (!hwpt->common.domain) {
>> + rc = -EINVAL;
>> + xa_unlock(&ictx->objects);
>> + goto out;
>> + }
>> +
>> + if (!iommufd_lu) {
>> + rc = check_iopt_pages_preserved(session, hwpt);
>
>This seems slightly redundant, we are calling check_iopt_pages_preserved
>for every single preserved HWPT. However, multiple HWPTs can share the
>same underlying IOAS.

Agreed. I think this check and the earlier immutable thing need to be
done at IOPT level. I think the rework for that should also remove the
redundant checks here.
>
>If a VMM has 5 devices sharing a single IOAS, this code will walk the
>exact same iopt_area_itree and validate the exact same memfd seals 5
>times. Should we perhaps track which iopts have already been validated
>during the loop to avoid this redundant tree traversal?
>
>> + if (rc) {
>> + xa_unlock(&ictx->objects);
>> + goto out;
>> + }
>> + } else if (iommufd_lu) {
>> + hwpts[nr_hwpts] = hwpt;
>> + hwpt_lu = &iommufd_lu->hwpts[nr_hwpts];
>> +
>> + hwpt_lu->token = hwpt->lu_token;
>> + hwpt_lu->reclaimed = false;
>> + }
>> +
>> + nr_hwpts++;
>> + }
>> + xa_unlock(&ictx->objects);
>> +
>> + if (WARN_ON(iommufd_lu && iommufd_lu->nr_hwpts != nr_hwpts)) {
>> + rc = -EFAULT;
>> + goto out;
>> + }
>> +
>> + if (iommufd_lu) {
>> + /*
>> + * iommu_domain_preserve may sleep and must be called
>> + * outside of xa_lock
>> + */
>> + for (i = 0; i < nr_hwpts; i++) {
>> + hwpt = hwpts[i];
>> + hwpt_lu = &iommufd_lu->hwpts[i];
>> +
>> + rc = iommu_domain_preserve(hwpt->common.domain, &domain_ser);
>> + if (rc < 0)
>> + goto out;
>> +
>> + hwpt_lu->domain_data = __pa(domain_ser);
>> + }
>> + }
>> +
>> + rc = nr_hwpts;
>> +
>> +out:
>> + kfree(hwpts);
>> + 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_lu *iommufd_lu;
>> + size_t serial_size;
>> + void *mem;
>> + int rc;
>> +
>> + if (IS_ERR(ictx))
>> + return PTR_ERR(ictx);
>> +
>> + rc = iommufd_save_hwpts(ictx, NULL, args->session);
>> + if (rc < 0)
>> + goto err_ioas_mutable;
>> +
>> + serial_size = struct_size(iommufd_lu, hwpts, rc);
>> +
>> + mem = kho_alloc_preserve(serial_size);
>> + if (!mem) {
>> + rc = -ENOMEM;
>> + goto err_ioas_mutable;
>> + }
>> +
>> + iommufd_lu = mem;
>> + iommufd_lu->nr_hwpts = rc;
>> + rc = iommufd_save_hwpts(ictx, iommufd_lu, args->session);
>
>We call iommufd_save_hwpts twice (first to count/validate & second to
>serialize). Because xa_lock is dropped between these two calls to
>allocate KHO memory, we have a TOCTOU race.

Agreed. Will handle this in next revision.
>
>If userspace concurrently creates or destroys a preserved HWPT during
>this window, nr_hwpts will change, and the second pass will hit:
>WARN_ON(iommufd_lu && iommufd_lu->nr_hwpts != nr_hwpts)
>
>I'm not sure if that's a situation to WARN? Also, what about kernels
>which have panic_on_warn?
>
>> + if (rc < 0)
>> + goto err_free;
>> +
>> + args->serialized_data = virt_to_phys(iommufd_lu);
>> + iommufd_ctx_put(ictx);
>> + return 0;
>> +
>> +err_free:
>> + kho_unpreserve_free(mem);
>> +err_ioas_mutable:
>> + iommufd_set_ioas_mutable(ictx);
>> + iommufd_ctx_put(ictx);
>> + return rc;
>> +}
>> +
>> +static int iommufd_liveupdate_freeze(struct liveupdate_file_op_args *args)
>> +{
>> + /* No-Op; everything should be made read-only */
>
>Is there a plan to populate this in a future series? If it's a permanent
>No-Op because the lu_map_immutable flag already handles the read-only
>enforcement during the preserve phase, we should probably update the
>comment to explicitly state that, rather than implying there is missing
>logic?

Agreed. Will update the comment.
>
>> + return 0;
>> +}
>> +
>
>[ ---- >8 ----- ]
>
>> diff --git a/include/linux/kho/abi/iommufd.h b/include/linux/kho/abi/iommufd.h
>> new file mode 100644
>> index 000000000000..f7393ac78aa9
>> --- /dev/null
>> +++ b/include/linux/kho/abi/iommufd.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +/*
>> + * Copyright (C) 2025, Google LLC
>> + * Author: Samiullah Khawaja <skhawaja@google.com>
>> + */
>> +
>> +#ifndef _LINUX_KHO_ABI_IOMMUFD_H
>> +#define _LINUX_KHO_ABI_IOMMUFD_H
>> +
>> +#include <linux/mutex_types.h>
>> +#include <linux/compiler.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * DOC: IOMMUFD Live Update ABI
>> + *
>> + * This header defines the ABI for preserving the state of an IOMMUFD file
>> + * across a kexec reboot using LUO.
>> + *
>> + * This interface is a contract. Any modification to any of the serialization
>> + * structs defined here constitutes a breaking change. Such changes require
>> + * incrementing the version number in the IOMMUFD_LUO_COMPATIBLE string.
>> + */
>> +
>> +#define IOMMUFD_LUO_COMPATIBLE "iommufd-v1"
>> +
>> +struct iommufd_hwpt_lu {
>> + u32 token;
>> + u64 domain_data;
>> + bool reclaimed;
>> +} __packed;
>> +
>
>Nit: Because of __packed, putting the u64 after the u32 forces an
>unaligned 8-byte access at offset 4. Also, it's generally safer to use
>explicitly sized types like u8 instead of bool for cross-kernel ABI
>structs to avoid any compiler-specific sizing ambiguities. (This is the
>reason most uAPI defs avoid using bool)
>
>We can achieve natural alignment without implicit padding by ordering it
>from largest to smallest and explicitly padding it out:
>
>struct iommufd_hwpt_lu {
> __u64 domain_data;
> __u32 token;
> __u8 reclaimed;
> __u8 padding[3];
>} __packed;
>
>This guarantees an exact 16-byte footprint with perfectly aligned 64-bit
>and 32-bit accesses.
>
>> +struct iommufd_lu {
>> + unsigned int nr_hwpts;
>> + struct iommufd_hwpt_lu hwpts[];
>> +};
>
>Same here regarding explicitly sized types and packing (as pointed by
>Vipin too)
>
>> +
>> +#endif /* _LINUX_KHO_ABI_IOMMUFD_H */
>
>Thanks,
>Praan