Re: [PATCH v2 12/16] iommufd: Implement ioctl to mark HWPT for preservation

[Samiullah Khawaja]

On Tue, May 19, 2026 at 11:05:02PM +0000, Pranjal Shrivastava wrote:
> On Mon, Apr 27, 2026 at 05:56:29PM +0000, Samiullah Khawaja wrote:
> > From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> >
> > Userspace provides a token to mark the HWPT for preservation. Note that
> > this token is not the LUO token that is used to preserve the iommufd.
> > Once all the required HWPT are marked for preservation, the user can
> > preserve the iommufd into LUO. The iommufd will preserve the HWPTs that
> > are marked for preservation.
> >
> > The marked HWPTs are tracked using a new XArray mark protected by a new
> > liveupdate mutex. This mutex will also be used during iommufd
> > preservation to protect against any race with the mark preserve ioctl.
> >
> > The HWPT token will be used during restore to identify this HWPT. The
> > restoration logic is not implemented and will be added later.
> >
> > Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > Signed-off-by: Samiullah Khawaja <skhawaja@xxxxxxxxxx>

[snip]
> > +int iommufd_hwpt_liveupdate_mark_preserve(struct iommufd_ucmd *ucmd)
> > +{
> > +	struct iommu_hwpt_liveupdate_mark_preserve *cmd = ucmd->cmd;
> > +	struct iommufd_hwpt_paging *hwpt_target;
> > +	struct iommufd_hwpt_paging *hwpt_paging;
> > +	struct iommufd_ctx *ictx = ucmd->ictx;
> > +	struct iommufd_object *obj;
> > +	unsigned long index;
> > +	int rc = 0;
> > +
> > +	hwpt_target = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
> > +	if (IS_ERR(hwpt_target))
> > +		return PTR_ERR(hwpt_target);
> > +
> > +	mutex_lock(&ictx->liveupdate_mutex);
> > +
> > +	xa_lock(&ictx->objects);
> > +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
> > +		if (WARN_ON_ONCE(obj->type != IOMMUFD_OBJ_HWPT_PAGING))
> > +			continue;
> > +
> > +		hwpt_paging = to_hwpt_paging(container_of(obj, struct iommufd_hw_pagetable, obj));
> > +		if (hwpt_paging->liveupdate_token == cmd->hwpt_token) {
> > +			rc = -EADDRINUSE;
> > +			goto out_unlock;
> > +		}
> > +	}
> > +
>
> Nit: What happens if the user calls this IOCTL on an HWPT that is already
> marked but passes a different token?
>
> The xa_for_each_marked loop will not match the new token (so it bypasses
> -EADDRINUSE case) and the code will silently overwrite the HWPT's
> existing token?
>
> Since there is no unmark / update UAPI, It can be a fair policy as well,
> but maybe we should call it out explicitly, maybe a log like:
> dev_warn("Overwriting token to %d ...") ?

Agreed. I will do this.
> If NOT, then we should avoid overwriting the token and scream with
> retval like -EINVAL & dev_err in dmesg.
>
> > +	__xa_set_mark(&ictx->objects, hwpt_target->common.obj.id, IOMMUFD_OBJ_LIVEUPDATE_MARK);
> > +	hwpt_target->liveupdate_token = cmd->hwpt_token;
> > +
> > +out_unlock:
> > +	xa_unlock(&ictx->objects);
> > +	mutex_unlock(&ictx->liveupdate_mutex);
> > +	iommufd_put_object(ictx, &hwpt_target->common.obj);
> > +	return rc;
> > +}
>
> With those two nits addressed,
>
> Reviewed-by: Pranjal Shrivastava <praan@xxxxxxxxxx>
>
> Thanks,
> Praan

Thanks,
Sami