Re: [PATCH v4 10/15] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `cond_resched()` in the PRI queue polling loop, potentially causing soft lockups.
--

commit 3323ab058351966ce0deb549fa5694bcbf787b2e
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

This commit converts PRIQ entries into iopf_faults for PRI-enabled masters
and reports them. It also discards PASID Stop Markers prior to fault
reporting and keeps the LAST-page DENY path for unknown StreamIDs.

> 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 43a6029cef430..c631c6344685d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -2380,9 +2381,47 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
>  	last = FIELD_GET(PRIQ_0_PRG_LAST, evt[0]);
>  	grpid = FIELD_GET(PRIQ_1_PRG_IDX, evt[1]);
>  
> -	dev_info(smmu->dev, "unexpected PRI request received:\n");
> -	dev_info(smmu->dev,
> -		 "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 0x%016llx\n",
> +	/*
> +	 * A PASID Stop Marker (LRW = 0b100) does not expect a response and
> +	 * must be discarded before fault reporting: see the documentation
> +	 * at iommu_report_device_fault().
> +	 */
> +	if (last && !(evt[0] & (PRIQ_0_PERM_READ | PRIQ_0_PERM_WRITE)))
> +		return;
> +
> +	mutex_lock(&smmu->streams_mutex);
[ ... ]

[Severity: High]
This is a pre-existing issue, but missing a cond_resched() inside the queue
draining loop could potentially cause soft lockups:

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:arm_smmu_priq_thread() {
	...
	do {
		while (!queue_remove_raw(q, evt))
			arm_smmu_handle_ppr(smmu, evt);

		if (queue_sync_prod_in(q) == -EOVERFLOW)
			dev_err(smmu->dev, "PRIQ overflow detected -- requests lost\n");
	} while (!queue_empty(llq));
	...
}

Could a faulty or malicious PCI device flood the PRI queue with requests
faster than the CPU can consume them? 

If the outer loop continuously polls the hardware producer index via
queue_sync_prod_in(), preventing queue_empty() from returning true, would
the host CPU get stuck in an infinite loop inside the IRQ thread?

Since arm_smmu_priq_thread() runs as a threaded IRQ handler, wouldn't this
starve other normal tasks on the CPU and lead to a soft lockup or RCU stall?

Does it make sense to include cond_resched() in this loop, similar to what
is done in arm_smmu_evtq_thread()?

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