Re: [PATCH v3 03/13] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Jonathan Cameron]

> When a device is switching away from a domain, either through a detach or a
> replace operation, in-flight stall events for the old domain might still be
> on the SMMU's hardware event queue or on the IOMMU core's IOPF queue. Thus,
> if the IOMMU core swaps the device's attach_handle and frees the old domain
> before those handlers complete, the IOPF work might hit use-after-free.
> 
> Two queues need to be drained: the SMMU hardware event queue and the IOMMU
> core IOPF software workqueue. Start with the former: add a counting-based
> arm_smmu_drain_queue() helper, and poll the evtq on a domain detach, so a
> pending IRQ won't let the threaded handler run after the drain and queue a
> fault referencing the domain being freed. Its until_empty mode serves the
> suspend and runtime PM routines that would drain the CMDQ. Any timed-out
> drain fires a WARN_ON as well, since reaching the timeout would take some
> stuck consumer in any realistic case.
> 
> The existing queue_poll() API is not reusable for such a drain: it is the
> atomic busy-wait for the command issuing paths, and it assumes a hardware
> consumer making progress. A drain caller is sleepable, in contrast, while
> the EVTQ/PRIQ consumer is a threaded IRQ handler that needs the CPU: such
> a busy wait would starve the handler throughout an entire timeout, whenever
> the waiter and the handler shared one CPU on a non-preemptible kernel. So,
> this new sleeping helper is marked with a might_sleep() as well, given that
> an atomic-context misuse would otherwise hide behind an empty queue.
> 
> Note that a drained event is dequeued, but not necessarily handled, since
> queue_remove_raw() moves the MMIO CONS before the threaded IRQ handler gets
> to push the event onto the IOPF workqueue. A subsequent change will invoke
> synchronize_irq() and iopf_queue_flush_dev() to close that gap, and it will
> act on the errno of a timed-out drain too.
> 
> The drain runs before the IOMMU core swaps the device's attach handle, so a
> fault event generated on the new STE during this window resolves to the old
> handle, completing with IOMMU_PAGE_RESP_INVALID that resumes the stall with
> abort: the impact is bounded to that one failed transaction.
> 
> Also run the drain for every stall-capable master, even when the departing
> attachment did not enable IOPF: such a stall event has to be aborted while
> it still resolves to the old attach handle, otherwise the threaded handler
> could pick it up right after the handle swap, mistakenly resuming it as if
> it were a valid page fault against a new domain.
>

Useful perhaps to call out if this has been seen in real systems or
not. I agree with the analysis but would rather hope drivers are
well behaved in ensuring all traffic is done, adn this is hardeninging
/ handling of naught hardware activity (all good if so!)

> Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
> Cc: stable@xxxxxxxxxxxxxxx # v6.16
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
>
> 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 e00b6c88214f..d255ff2519f9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -948,6 +948,86 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
>  					   cmds->num, true);
>  }
>  
> +/**
> + * arm_smmu_drain_queue - Drain an SMMU queue
> + * @smmu: the SMMU device
> + * @q: the queue to drain
> + * @until_empty: target selection
> + *
> + * With @until_empty == true (for CMDQ), exit once the queue is observed empty:
> + *
> + *   cons0                cons                                prod
> + *     |                   |                                   |
> + *  ---+###################+=====================+=============+--->
> + *                         |<--------- undrained==0? --------->|
                                                    ^
What is the + indicating?  Seems where prod0 that isn't relevant here
would have been - that is a little confusing so maybe drop?

> + *
> + * With @until_empty == false (for EVTQ/PRIQ), exit once "drained" reaches its
> + * target: "pending" (i.e. prod0 - cons0, frozen at the entry time):
> + *
> + *   cons0                cons                 prod0         (prod)
> + *     |<---- drained ---->|                     |             |
> + *  ---+###################+=====================+=============+--->
> + *     |<--------------- pending --------------->|
> + *
> + * Note that a drained entry is dequeued, but not necessarily handled: the
> + * EVTQ/PRIQ callers must follow up with a synchronize_irq() to wait for the
> + * threaded IRQ handler to finish handling the dequeued entries.
> + *
> + * Context: Process context; may sleep.
> + * Return: 0 on success or a negative errno on timeout.
> + */
> +static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
> +				struct arm_smmu_queue *q, bool until_empty)

That name suggests this is doing the draining rather than waiting
for it to happen elsewhere.

> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> +	u32 cons, prod, prev, undrained;
> +	u32 drained = 0, pending;

Pet irritation. Prefer splitting the elements that assign and those
that don't onto seeprate lines.  Here that just means moving pending
up one line.

> +
> +	might_sleep();
> +
> +	cons = readl_relaxed(q->cons_reg);
> +	prod = readl_relaxed(q->prod_reg);
> +	/* The exit target: the number of entries in the queue at entry */
> +	pending = Q_POS(&q->llq, prod - cons);

Applying a macro called Q_POS to a difference is a bit confusing to
me given the output isn't a position of anything. Maybe just needs
a wrapper Q_DIFF(q->llq, prod, cons)  Can use Q_POS underneath
but avoid that naming out here well away from the macro definitions.

> +
> +	while (true) {

Maybe pull defintion of prev and undrained in here so it is clear
they aren't state maintained across iternations.

> +		/* Accumulate the entries consumed since the last poll */
> +		prev = cons;
> +		cons = readl_relaxed(q->cons_reg);
> +		drained += Q_POS(&q->llq, cons - prev);
> +
> +		prod = readl_relaxed(q->prod_reg);
> +		undrained = Q_POS(&q->llq, prod - cons);
> +
> +		/* Exit on an empty queue, regardless of until_empty */
> +		if (!undrained)

Given you don't use undrained again (maybe in later patches, in which
case ignore me.)
		if (Q_DIFF(&q->llq, prod, cons) == 0)
perhaps.  This one entirely up to you as maybe the named local does
help with readability a little.

> +			return 0;
> +
> +		/* Snapshot mode: exit once the pending entries are drained */
> +		if (!until_empty && drained >= pending)
> +			return 0;
> +
> +		/*
> +		 * A timeout means the consumer might be stuck. In theory, if it
> +		 * moves 2 * qsize entries or more within a single poll interval
> +		 * Q_POS() would wrap and undercount drained: that could trigger
> +		 * a spurious warning too, if the queue was never once observed
> +		 * empty. Yet, that much consumption in such a short interval is
> +		 * unrealistic. WARN it only, as a stuck consumer is a real bug.

I don't like 'unrealisitic' based defenses (even though I agree it is pretty
unlikely).  Is there a way to bound this?  Maybe future systems will
be much quicker.

> +		 */
> +		if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))

Why WARN_ON here then a dev_warn_ratelimited() below? 

> +			break;
> +
> +		/* The consumer might be a threaded IRQ handler. Yield to it */
> +		usleep_range(100, 200);

fsleep() perhaps then we don't get to argue why that slack.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>