[PATCH v3 04/13] iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

[Jonathan Cameron]

> arm_smmu_attach_release() polls the hardware event queue to drain all the
> in-flight stall events for an old domain that the IOMMU core might free at
> any moment. However, a drained event is dequeued, yet it is not necessarily
> handled, and the IOPF work for a handled one can still be running.
> 
> So, first synchronize_irq() on the evtq and the combined IRQs following the
> drain, in order to guarantee that every dequeued event has reached the IOPF
> workqueue, since synchronize_irq() waits for an in-flight IRQ thread. Skip
> the waits on a timed-out drain though, since a stuck consumer would block

drop the "though" - doesn't read well or add anything.

> the unbounded wait_event() inside synchronize_irq() as well.
> 
> Then, invoke iopf_queue_flush_dev() to finally drain the IOPF workqueue, as
> the fault work references the old domain via its attach handle, while the
> IOMMU core might free that old domain once the detach call returns.
> 
> If arm_smmu_drain_queue() times out, fault work may still be in flight, and
> iopf_queue_remove_device() would free iopf groups that the queued work also
> references. Skip the iopf teardown and leak the master_domain, rather than
> risk a use-after-free.
> 
> The skip also leaks the iopf refcount, keeping the device enrolled on the
> IOPF queue, which would strand its fault parameter on the queue list once
> the device teardown frees dev->iommu, crashing a later iopf_queue_free().
> Reclaim the enrollment in arm_smmu_release_device(), where all the attach
> handles are gone so a straggler report cannot queue a new fault group.
> 
> Note that a residual race window remains between an iopf_queue_flush_dev()
> and iopf_queue_remove_device(): a fault arriving in between still resolves
> to the old attach handle, as the IOMMU core publishes a handle change only
> after the driver ops return. This window predates the drain narrowing it,
> and is only closable by an ordering fix in the IOMMU core. Furthermore, a
> timed-out drain shares exactly the same window, given that it must keep the
> device enrolled on the IOPF queue, where iopf_queue_remove_device() would
> free the iopf groups that any in-flight fault work still references.
> 
> Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
> Cc: stable at vger.kernel.org # v6.16
> Co-developed-by: Barak Biber <bbiber at nvidia.com>
> Signed-off-by: Barak Biber <bbiber at nvidia.com>
> Co-developed-by: Stefan Kaestle <skaestle at nvidia.com>
> Signed-off-by: Stefan Kaestle <skaestle at nvidia.com>
> Signed-off-by: Malak Marrid <mmarrid at nvidia.com>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc at nvidia.com>
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index d255ff2519f9..1851ee7ec15b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3399,6 +3399,7 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
>  	struct arm_smmu_master *master = state->master;
>  	struct arm_smmu_device *smmu = master->smmu;
> +	int ret = 0;
>  
>  	lockdep_assert_not_held(&arm_smmu_asid_lock);
>  	iommu_group_mutex_assert(master->dev);
> @@ -3412,8 +3413,40 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  	 * up once the IOMMU core swaps the handle, mistakenly resuming it
>  	 * against the next domain.
>  	 */
> -	if (master->stall_enabled)
> -		arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
> +	if (master->stall_enabled) {
> +		ret = arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
> +		/*
> +		 * Ensure pending events have reached the IOPF queue, unless
> +		 * the drain timed out: a stuck consumer would also block an
> +		 * unbounded wait_event() inside the synchronize_irq().
> +		 */
> +		if (!ret && smmu->evtq.q.irq)
> +			synchronize_irq(smmu->evtq.q.irq);
> +		/* Pending events might be in the combined_irq handler */
> +		if (!ret && smmu->combined_irq)
> +			synchronize_irq(smmu->combined_irq);

I'd group the two things that happen if we didn't time out.

		if (!ret) { /* Didn't time out */
			if (smmu->evtq.q.irq)
				synchronize_irq(smmu->evt.q.irq);
			if (smmu->combined_irq)
				synchronize_irq(smmu->combined_irq);
		}
		
> +	}

Thanks,
Jonathan

-- 
Jonathan Cameron <jonathan.cameron at oss.qualcomm.com>