On Tue, Mar 22, 2022 at 04:15:44PM -0600, Alex Williamson wrote:
> > +static struct iopt_area *
> > +iopt_alloc_area(struct io_pagetable *iopt, struct iopt_pages *pages,
> > + unsigned long iova, unsigned long start_byte,
> > + unsigned long length, int iommu_prot, unsigned int flags)
> > +{
> > + struct iopt_area *area;
> > + int rc;
> > +
> > + area = kzalloc(sizeof(*area), GFP_KERNEL);
> > + if (!area)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + area->iopt = iopt;
> > + area->iommu_prot = iommu_prot;
> > + area->page_offset = start_byte % PAGE_SIZE;
> > + area->pages_node.start = start_byte / PAGE_SIZE;
> > + if (check_add_overflow(start_byte, length - 1, &area->pages_node.last))
> > + return ERR_PTR(-EOVERFLOW);
> > + area->pages_node.last = area->pages_node.last / PAGE_SIZE;
> > + if (WARN_ON(area->pages_node.last >= pages->npages))
> > + return ERR_PTR(-EOVERFLOW);
>
> @area leaked in the above two error cases.
Yes, thanks
> > +int iopt_access_pages(struct io_pagetable *iopt, unsigned long iova,
> > + unsigned long length, struct page **out_pages, bool write)
> > +{
> > + unsigned long cur_iova = iova;
> > + unsigned long last_iova;
> > + struct iopt_area *area;
> > + int rc;
> > +
> > + if (!length)
> > + return -EINVAL;
> > + if (check_add_overflow(iova, length - 1, &last_iova))
> > + return -EOVERFLOW;
> > +
> > + down_read(&iopt->iova_rwsem);
> > + for (area = iopt_area_iter_first(iopt, iova, last_iova); area;
> > + area = iopt_area_iter_next(area, iova, last_iova)) {
> > + unsigned long last = min(last_iova, iopt_area_last_iova(area));
> > + unsigned long last_index;
> > + unsigned long index;
> > +
> > + /* Need contiguous areas in the access */
> > + if (iopt_area_iova(area) < cur_iova || !area->pages) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Should this be (cur_iova != iova && iopt_area_iova(area) < cur_iova)?
Oh good eye
That is a typo < should be >:
if (iopt_area_iova(area) > cur_iova || !area->pages) {
There are three boundary conditions here to worry about
- interval trees return any nodes that intersect the queried range
so the first found node can start after iova
- There can be a gap in the intervals
- The final area can end short of last_iova
The last one is botched too and needs this:
for ... { ...
}
+ if (cur_iova != last_iova)
+ goto out_remove;
The test suite isn't covering the boundary cases here yet, I added a
FIXME for this for now.
Thanks,
Jason
