Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "Liu, Yi L" <yi.l.liu@intel.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Tony Krowiak <akrowiak@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Jason Herne <jjherne@linux.ibm.com>
Subject: Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable
Date: Thu, 24 Nov 2022 08:59:24 -0400	[thread overview]
Message-ID: <Y39qrCtw0d0dfbLt@nvidia.com> (raw)
In-Reply-To: <BN9PR11MB5276E07F9CB1A006FAC9E4098C0F9@BN9PR11MB5276.namprd11.prod.outlook.com>

On Thu, Nov 24, 2022 at 07:08:06AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, November 23, 2022 9:49 PM
> > 
> > From: Matthew Rosato <mjrosato@linux.ibm.com>
> > 
> > vfio_iommufd_bind() creates an access which has an unmap callback, which
> > can be called immediately. So dma_unmap() callback should tolerate the
> > unmaps that come before the emulated device is opened.
> > 
> > To achieve above, vfio_ap_mdev_dma_unmap() needs to validate that
> > unmap
> > request matches with one or more of these stashed values before
> > attempting unpins.
> > 
> > Currently, each mapped iova is stashed in its associated vfio_ap_queue;
> > Each stashed iova represents IRQ that was enabled for a queue. Therefore,
> > if a match is found, trigger IRQ disable for this queue to ensure that
> > underlying firmware will no longer try to use the associated pfn after
> > the page is unpinned. IRQ disable will also handle the associated unpin.
> > 
> > Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> > Cc: Halil Pasic <pasic@linux.ibm.com>
> > Cc: Jason Herne <jjherne@linux.ibm.com>
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> > b/drivers/s390/crypto/vfio_ap_ops.c
> > index bb7776d20792..62bfca2bbe6d 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -1535,13 +1535,35 @@ static int vfio_ap_mdev_set_kvm(struct
> > ap_matrix_mdev *matrix_mdev,
> >  	return 0;
> >  }
> > 
> > +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
> > u64 length)
> > +{
> > +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
> > +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
> > +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
> > +	struct vfio_ap_queue *q;
> > +	int loop_cursor;
> > +	u64 pfn;
> > +
> > +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> > +		pfn = q->saved_iova >> PAGE_SHIFT;
> > +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
> > +			vfio_ap_irq_disable(q);
> > +			break;
> 
> does this need a WARN_ON if the length is more than one page?

The iova and length are the range being invalidated, the driver has no
control over them and length is probably multiple pages.

But this test doesn't look right?

   if (iova > q->saved_iova && q->saved_iova < iova + length)

Since the page was pinned we can assume iova and length are already
PAGE_SIZE aligned.

Jason