Re: [RFC PATCH v2 06/11] iommufd: Map memory provider files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iommufd: 32-bit truncation of file offset breaks provider revokes
- [Medium] iommufd: missing cond_resched() in large IOVA mapping/unmapping loops
--

commit b6da636b8b49032486fd2a6044789fc9bf757f2b
Author: Fred Griffoul <fgriffo@amazon.co.uk>

iommufd: Map memory provider files

This patch adds support for mapping memory provider files into iommufd 
domains. It bypasses pinning and accounting, and allows the provider to 
dynamically back or revoke memory ranges while keeping mappings in sync.

> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
> index ba539bfaac317..80d926c86c6cf 100644
> --- a/drivers/iommu/iommufd/io_pagetable.h
> +++ b/drivers/iommu/iommufd/io_pagetable.h
> @@ -207,6 +213,14 @@ struct iopt_pages_dmabuf {
>  	unsigned long start;
>  };
>  
> +struct iopt_pages_provider {
> +	struct mem_provider_attachment att;
> +	/* Byte offset in the provider file, always PAGE_SIZE aligned */
> +	unsigned long start;

[Severity: High]
The UAPI structure iommu_ioas_map_file defines the file start offset as a
64-bit __aligned_u64, but it is stored here as an unsigned long. On 32-bit
systems, does this silently truncate offsets larger than 4GB?

> +	/* Charged for the page tables that a revoke allocates */
> +	struct mem_cgroup *memcg;
> +};
> +
>  /*
>   * This holds a pinned page list for multiple areas of IO address space. The
>   * pages always originate from a linear chunk of userspace VA. Multiple

[ ... ]

> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index 660af3d46f5e1..aa7740326ad6a 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -238,6 +240,35 @@ static void iommu_unmap_nofail(struct iommu_domain *domain, unsigned long iova,
>  	WARN_ON(ret != size);
>  }
>  
> +/*
> + * Provider pages are mapped with PAGE_SIZE entries, and may have holes.  Some
> + * IOMMU drivers warn when asked to unmap an IOVA that is not mapped, so find
> + * the runs that are mapped and unmap each one, without touching the holes.
> + * iopt_provider_map() refuses frame 0, which iova_to_phys() reports for
> + * an IOVA that is not mapped.
> + */
> +static void iopt_provider_unmap(struct iopt_area *area,
> +				struct iommu_domain *domain,
> +				unsigned long start_index,
> +				unsigned long last_index)
> +{
> +	unsigned long iova = iopt_area_index_to_iova(area, start_index);
> +	size_t left = (last_index - start_index + 1) * PAGE_SIZE;
> +
> +	while (left) {
> +		size_t len = 0;
> +
> +		while (len < left && iommu_iova_to_phys(domain, iova + len))
> +			len += PAGE_SIZE;

[Severity: Medium]
Memory provider files can be extremely large. Since this loop unmaps the 
entire range in a single pass without explicitly yielding the CPU, could 
this cause soft lockups on kernels without forced preemption? Would it be
better to add a cond_resched() here?

> +		if (len)
> +			iommu_unmap_nofail(domain, iova, len);
> +		/* Step over the run and the hole page that ended it. */
> +		len = min(len + PAGE_SIZE, left);
> +		iova += len;
> +		left -= len;
> +	}
> +}

[ ... ]

> @@ -1647,6 +1687,228 @@ void iopt_pages_untrack_all_domains(struct iopt_area *area,
>  	}
>  }
>  
> +static int iopt_provider_prot(struct iopt_area *area, u32 attrs)
> +{
> +	int prot = area->iommu_prot;
> +
> +	if (attrs & MEM_PROVIDER_ATTR_READONLY)
> +		prot &= ~IOMMU_WRITE;
> +	if (mem_provider_type(attrs) != MEM_PROVIDER_TYPE_RAM) {
> +		prot &= ~IOMMU_CACHE;
> +		prot |= IOMMU_MMIO;
> +	}
> +	return prot;
> +}
> +
> +/*
> + * Map what the provider backs in [start_index, last_index] of the area into
> + * @domain, and leave the holes unmapped.  Each frame is mapped with a
> + * PAGE_SIZE entry, so that a revoke of part of a large block never has to
> + * split a larger IOMMU page.  On failure nothing in the range is mapped.
> + */
> +static int iopt_provider_map(struct iopt_area *area, struct iopt_pages *pages,
> +			     struct iommu_domain *domain,
> +			     unsigned long start_index,
> +			     unsigned long last_index)
> +{
> +	unsigned long base = pages->provider.start >> PAGE_SHIFT;
> +	unsigned long index = start_index;
> +	int rc;
> +
> +	lockdep_assert_held(&pages->mutex);
> +
> +	if ((1UL << __ffs(domain->pgsize_bitmap)) > PAGE_SIZE)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * A revoke unmaps under pages->mutex only, so it would race with a
> +	 * dirty bitmap read, and lose the dirty bits of what it unmaps.
> +	 * Provider pages are not mapped in a domain with dirty tracking.
> +	 */
> +	if (domain->dirty_ops)
> +		return -EOPNOTSUPP;
> +
> +	while (index <= last_index) {
> +		unsigned long pfn, iova, block_end, nr, i;
> +		int order = PUD_ORDER;
> +		u32 attrs;
> +		int prot;
> +
> +		rc = mem_provider_get_page(&pages->provider.att, base + index,
> +					   &pfn, &order, &attrs);

[Severity: Medium]
Similarly, mapping a very large provider file could translate to processing
millions of frames in one go. Should this loop also yield the CPU to prevent
soft lockups?

> +		if (rc == -EFAULT) {
> +			index++;
> +			continue;
> +		}

[ ... ]

> +/*
> + * The provider changed the frames behind [offset, offset + len).  In every
> + * domain, unmap the range and map what the provider backs now.  A device that
> + * accesses the range in between faults.
> + */
> +static void iopt_provider_revoke(struct mem_provider_attachment *att,
> +				 loff_t offset, loff_t len)
> +{
> +	struct iopt_pages *pages =
> +		container_of(att, struct iopt_pages, provider.att);
> +	u64 start = pages->provider.start;
> +	u64 end = start + (u64)pages->npages * PAGE_SIZE;
> +	struct iopt_pages_track *track;
> +	struct mem_cgroup *old_memcg;
> +	unsigned long first, last;
> +	int rc;
> +
> +	if (offset < 0 || len <= 0 || offset >= end || offset + len <= start)
> +		return;

[Severity: High]
Because pages->provider.start is stored as an unsigned long, it will wrap 
on 32-bit systems for offsets larger than 4GB.

When the provider later calls iopt_provider_revoke() with a 64-bit loff_t 
offset, this bounds check will compare the correct 64-bit offset against
the wrapped 32-bit start/end values. 

Will this cause the bounds check to evaluate to true (e.g. offset >= end) 
and abort early, bypassing the unmap and leaving the device with access to 
revoked memory?

> +	first = (max_t(u64, offset, start) - start) >> PAGE_SHIFT;
> +	last = (min_t(u64, offset + len, end) - start - 1) >> PAGE_SHIFT;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014093733.90312-1-griffoul@gmail.com?part=6