Re: [PATCH v4 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 9f62bb971f89fec6afb029cd8bd570abb08a5696
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Treat !mapped as an error

This commit treats a zero-byte mapping result in __do_map() as an error so
that the caller can mark the VM as unusable rather than silently mapping
nothing.

> 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;
> +
>  			if (ret) {
>  				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
>  				return -EINVAL;

[Severity: Medium]
As reported by Sashiko on v3 [1], can __do_map() in
drivers/gpu/drm/msm/msm_iommu.c still silently return 0 without mapping the
requested range when sg_table entries have zero or short DMA lengths?

The new check is inside the inner while (size) loop, where size is already
non-zero (and where ops->map() returning ret == 0 with mapped == 0 would
previously have spun in an infinite loop rather than returning early; note
also that setting ret = -ENOMEM is immediately overwritten by returning
-EINVAL).

However, when an sg_table entry has size = sg_dma_len(sg) == 0 (for example,
an unmapped sg_table when CONFIG_NEED_SG_DMA_LENGTH is enabled), the earlier
check in __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;
		}

evaluates 0 <= off as true for unsigned size_t off and continues past the
while (size) loop without ever reaching the !mapped check.

Similarly, if sgt->nents is 0 or the total sg_dma_len(sg) across sgt is less
than off + len, the outer for_each_sgtable_dma_sg() loop finishes with
len > 0 and __do_map() unconditionally returns 0 at the end of the function:

			if (ret) {
				__do_unmap(pgsize_bitmap, iova, addr - iova, arg, ops);
				return -EINVAL;
			}
		}
	}

	return 0;
}

without calling __do_unmap() on any partially mapped range or returning an
error to mark the VM as unusable.

Should __do_map() check whether len != 0 after the for_each_sgtable_dma_sg()
loop (unmapping [iova, addr) and returning an error if so), and should the
sibling while (len) loop in msm_iommu_pagetable_map_prr() also guard against
!ret && !mapped?

[1] https://lore.kernel.org/r/20261027230452.130031-7-robin.clark@oss.qualcomm.com

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