Re: [PATCH v3 6/6] drm/msm: Treat !mapped as an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm: __do_map() still returns 0 when sg_table maps nothing or short len
--

commit 363087af17cd51ea419c705b08215e2c785556f3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Treat !mapped as an error

This commit treats zero mapped bytes after an ops->map() call in __do_map()
as -ENOMEM so that mapping nothing is handled as an error rather than
silently ignored.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index c2950d32c5075..255972323f9a9 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -186,6 +186,10 @@ __do_map(unsigned long pgsize_bitmap, u64 iova, struct sg_table *sgt, size_t off
>  			size -= mapped;
>  			len  -= mapped;
>  
> +			/* If nothing was mapped, consider it an ENOMEM. */
> +			if (!ret && !mapped)
> +				ret = -ENOMEM;

[Severity: Medium]
Can __do_map() in drivers/gpu/drm/msm/msm_iommu.c still silently return 0
without mapping anything (or after mapping fewer than len bytes) when called
via vm_map_op() -> msm_iommu_pagetable_map() or msm_iommu_map()?

The new !mapped check is placed inside the inner while (size) loop, which
only executes when size = sg_dma_len(sg) > 0 (and where ret == 0 with
mapped == 0 would previously have caused an infinite loop rather than
silently returning).

However, if sg_dma_len(sg) == 0 (for example, when an sg_table is not
DMA-mapped, or analogous to the sg->length == 0 case under
CONFIG_DMABUF_DEBUG before commit b6b3547f9503 ("drm/msm: Convert map helper
to use dma-address")), the outer loop in __do_map() skips the inner while
(size) loop altogether because 0 <= off is always true for unsigned size_t:

drivers/gpu/drm/msm/msm_iommu.c:__do_map() {
	for_each_sgtable_dma_sg (sgt, sg, i) {
		size_t size = sg_dma_len(sg);
		phys_addr_t phys = sg_dma_address(sg);

		if (!len)
			break;

		if (size <= off) {
			off -= size;
			continue;
		}
		...
		while (size) {
			...
		}
	}

	return 0;
}

Similarly, if sgt->nents == 0 or the total DMA length across sgt is smaller
than off + len (which msm_iommu_map() checked via return (ret == len) ? 0 :
-EINVAL before commit efc83727eef1 ("drm/msm: Convert iommu map/unmap to
helpers")), for_each_sgtable_dma_sg() finishes with len > 0 and __do_map()
still returns 0 without unmapping addr - iova via __do_unmap() or marking
the VM unusable.

Should __do_map() also check if len != 0 after the for_each_sgtable_dma_sg()
loop and unmap any partial range before returning an error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027230452.130031-1-robin.clark@oss.qualcomm.com?part=6