> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Thursday, March 24, 2022 2:16 AM
>
> On Tue, Mar 22, 2022 at 04:15:44PM -0600, Alex Williamson wrote:
>
> > > +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.
>
Another nit about below:
+ /*
+ * Can't cross areas that are not aligned to the system page
+ * size with this API.
+ */
+ if (cur_iova % PAGE_SIZE) {
+ rc = -EINVAL;
+ goto out_remove;
+ }
Currently it's done after iopt_pages_add_user() but before cur_iova
is adjusted, which implies the last add_user() will not be reverted in
case of failed check here.
suppose this should be checked at the start of the loop.
Thanks
Kevin
