Re: [PATCH v4 04/15] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing final condition check after timeout evaluation in `arm_smmu_wait_for_queue_drained` causes false timeouts.
--

commit 69f47c0ae54518e91f78d4c86bc275dd8b087d48
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

When a device leaves a domain, fault events for the old domain may remain
in the SMMU event queue or the IOPF workqueue. If the IOMMU core frees that
domain before those events are handled, the work may use freed memory.
This commit starts with the hardware queue by using
arm_smmu_wait_for_queue_drained() to poll the EVTQ when an IOPF-enabled
attachment ends.

> 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 b908a8af31442..ef1fddad7868e 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,89 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
[ ... ]
> +static int arm_smmu_wait_for_queue_drained(struct arm_smmu_device *smmu,
> +					   struct arm_smmu_queue *q,
> +					   bool until_empty)
> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> +	u32 cons, prod, pending;
> +	u32 drained = 0;
> +
> +	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_DIFF(&q->llq, cons, prod);
> +
> +	while (true) {
> +		u32 prev, undrained;
> +
> +		/* Accumulate the entries consumed since the last poll */
> +		prev = cons;
> +		cons = readl_relaxed(q->cons_reg);
> +		drained += Q_DIFF(&q->llq, prev, cons);
> +
> +		prod = readl_relaxed(q->prod_reg);
> +		undrained = Q_DIFF(&q->llq, cons, prod);
> +
> +		/* Exit on an empty queue, regardless of until_empty */
> +		if (!undrained)
> +			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_DIFF() will 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.
> +		 */
> +		if (ktime_compare(ktime_get(), timeout) > 0)
> +			break;

[Severity: Medium]
Could this result in a false timeout if the task is preempted or delayed just
before the timeout check?

If execution is interrupted for a duration exceeding the timeout *after*
reading the hardware registers and evaluating the exit conditions, but
*before* checking the timeout:

1. The hardware might successfully drain the queue while the task is
   preempted.
2. When the task resumes, ktime_compare() returns true, and the loop breaks.
3. The function returns -ETIMEDOUT unconditionally without re-evaluating the
   hardware state.

Standard kernel polling macros (like read_poll_timeout) typically perform a
final re-evaluation of the exit conditions upon timeout to handle such
scheduling delays.

A false timeout here would lead to skipping the teardown and leaking the
domain to prevent a Use-After-Free, as handled in a subsequent commit
("iommu/arm-smmu-v3: Flush in-flight fault work on domain detach").

> +
> +		/* The consumer might be a threaded IRQ handler. Yield to it */
> +		fsleep(100);
> +	}
> +
> +	dev_warn_ratelimited(smmu->dev,
> +			     "queue drain timed out at prod=0x%x cons=0x%x\n",
> +			     prod, cons);
> +	return -ETIMEDOUT;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789081084.git.nicolinc@nvidia.com?part=4