Re: [PATCH 1/2] vfio/ccw: Add length to DMA_UNMAP checks

[Eric Farman <farman@linux.ibm.com>]

On Tue, 2022-07-26 at 12:12 -0400, Matthew Rosato wrote:
> On 7/26/22 11:01 AM, Eric Farman wrote:
> > As pointed out with the simplification of the
> > VFIO_IOMMU_NOTIFY_DMA_UNMAP notifier [1], the length
> > parameter was never used to check against the pinned
> > pages.
> > 
> > Let's correct that, and see if a page is within the
> > affected range instead of simply the first page of
> > the range.
> > 
> > [1] 
> > https://lore.kernel.org/kvm/20220720170457.39cda0d0.alex.williamson@redhat.com/
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
> >   drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
> >   drivers/s390/cio/vfio_ccw_ops.c |  2 +-
> >   3 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > b/drivers/s390/cio/vfio_ccw_cp.c
> > index 8963f452f963..f15b5114abd1 100644
> > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > @@ -170,12 +170,14 @@ static void page_array_unpin_free(struct
> > page_array *pa, struct vfio_device *vde
> >   	kfree(pa->pa_iova);
> >   }
> >   
> > -static bool page_array_iova_pinned(struct page_array *pa, unsigned
> > long iova)
> > +static bool page_array_iova_pinned(struct page_array *pa, unsigned
> > long iova,
> > +				   unsigned long length)
> >   {
> >   	int i;
> >   
> >   	for (i = 0; i < pa->pa_nr; i++)
> > -		if (pa->pa_iova[i] == iova)
> > +		if (pa->pa_iova[i] >= iova &&
> > +		    pa->pa_iova[i] <= iova + length)
> 
> For the sake of completeness, I think you want to be checking to
> make 
> sure the end of the page is also within the range, not just the
> start?
> 
> if (pa->pa_iova[i] >= iova &&
>      pa->pa_iova[i] + PAGE_SIZE <= iova + length)

Well +PAGE_SIZE would iterate to the next page, so that would be
captured on the next iteration of the for(i) loop if the pages were
contiguous (or not applicable, if the pages weren't).

But, since the comment is really about the end of the page (0xfff), I
guess I'm not understanding what that gets us so perhaps you could help
elaborate your question? From my chair, since the pa_iova argument
passed to vfio_pin_pages() pins the whole page, checking the start
address versus the end (or anywhere in between) should still capture
its interaction with an affected range. That is to say, we don't care
about the -whole- page being within the unmap range, but -any- part of
it.