Re: [PATCH v5 10/18] iommu/arm-smmu-v3: Recheck CMDQ_ERR in tegra241_vintf0_handle_error()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition between teardown paths and the threaded ISR leading to a NULL pointer dereference.
- [High] Missing index offset for the second 64-bit LVCMDQ error map causes incorrect array access and an unhandled IRQ storm.
- [High] Premature IRQ registration allows the ISR to fire before core data structures are allocated, leading to an uninitialized pointer dereference.
--

commit 24d0e50ef454f78b71fd6e45f50c5e36ef3cc631
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Recheck CMDQ_ERR in tegra241_vintf0_handle_error()

This patch introduces locking when reading and acknowledging the
GERROR/GERRORN pair in both tegra241_vintf0_handle_error() and
tegra241_vcmdq_hw_deinit(). It ensures that concurrent acknowledgments
do not skip errors on already-handled errors. It also properly
initializes the cmdq_err_lock at allocation.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 9012ab584d1dd..666dd23b0c7ca 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -319,10 +319,19 @@ static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
>  		while (map) {
>  			unsigned long lidx = __ffs64(map);
>  			struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[lidx];

[Severity: High]
This isn't a bug introduced by this patch, but does this code compute the
correct array index for logical VCMDQs when the outer loop index i is
greater than 0?

Since this loop processes LVCMDQ_ERR_MAP_64(i), if an error occurs on a logical
VCMDQ with an index >= 64, the hardware sets the corresponding bit in the
second 64-bit error map. By computing lidx purely as __ffs64(map) and omitting
an (i * 64) offset, wouldn't it erroneously access an index in the 0..63 range
instead of the correct queue? Could this fail to acknowledge the actual queue's
hardware error and lead to an unhandled IRQ storm?

[Severity: High]
This is a pre-existing issue, but is it possible for a race condition to occur
between the teardown paths and this threaded ISR?

If the driver is removed (e.g., via rmmod) and tegra241_cmdqv_remove() initiates
teardown and nullifies the vintf->lvcmdqs[lidx] pointers before free_irq() is
called, could this concurrently active ISR thread read the hardware map and
dereference the newly nullified vintf->lvcmdqs[lidx] without holding any locks?

> -			u32 gerror = readl_relaxed(REG_VCMDQ_PAGE0(vcmdq, GERROR));
> +			struct arm_smmu_cmdq *cmdq = &vcmdq->cmdq;
> +			unsigned long flags;
> +			u32 gerror, gerrorn;

[ ... ]

I also noticed another pre-existing issue outside the scope of this diff:

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c:__tegra241_cmdqv_probe() {
    ...
		ret = request_threaded_irq(irq, NULL, tegra241_cmdqv_isr,
					   IRQF_ONESHOT, "tegra241-cmdqv",
					   cmdqv);
    ...
}

[Severity: High]
This is a pre-existing issue, but is there a race condition here where the IRQ
handler is registered before core data structures are fully initialized?

If request_threaded_irq() is called while cmdqv->vintfs is still an
uninitialized pointer (e.g. following devm_krealloc without __GFP_ZERO), could a
latched interrupt cause the ISR to fire instantly and dereference
cmdqv->vintfs[0] in tegra241_cmdqv_isr() before it is safely initialized?

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