Re: [PATCH RFC v2 00/13] IOMMUFD Generic interface

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: "Eric Auger" <eric.auger@redhat.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"Rodel, Jorg" <jroedel@suse.de>,
	"Lu Baolu" <baolu.lu@linux.intel.com>,
	"Chaitanya Kulkarni" <chaitanyak@nvidia.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	"Daniel Jordan" <daniel.m.jordan@oracle.com>,
	"David Gibson" <david@gibson.dropbear.id.au>,
	"Eric Farman" <farman@linux.ibm.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"Jason Wang" <jasowang@redhat.com>,
	"Jean-Philippe Brucker" <jean-philippe@linaro.org>,
	"Martins, Joao" <joao.m.martins@oracle.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Matthew Rosato" <mjrosato@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Nicolin Chen" <nicolinc@nvidia.com>,
	"Niklas Schnelle" <schnelle@linux.ibm.com>,
	"Shameerali Kolothum Thodi"
	<shameerali.kolothum.thodi@huawei.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	"Keqian Zhu" <zhukeqian1@huawei.com>,
	"Steve Sistare" <steven.sistare@oracle.com>,
	"libvir-list@redhat.com" <libvir-list@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Laine Stump" <laine@redhat.com>
Subject: Re: [PATCH RFC v2 00/13] IOMMUFD Generic interface
Date: Wed, 21 Sep 2022 15:44:24 -0300	[thread overview]
Message-ID: <YytbiCx3CxCnP6fr@nvidia.com> (raw)
In-Reply-To: <20220921120649.5d2ff778.alex.williamson@redhat.com>

On Wed, Sep 21, 2022 at 12:06:49PM -0600, Alex Williamson wrote:

> > I still think the compat gaps are small. I've realized that
> > VFIO_DMA_UNMAP_FLAG_VADDR has no implementation in qemu, and since it
> > can deadlock the kernel I propose we purge it completely.
> 
> Steve won't be happy to hear that, QEMU support exists but isn't yet
> merged.

If Steve wants to keep it then someone needs to fix the deadlock in
the vfio implementation before any userspace starts to appear. 

I can fix the deadlock in iommufd in a terrible expensive way, but
would rather we design a better interface if nobody is using it yet. I
advocate for passing the memfd to the kernel and use that as the page
provider, not a mm_struct.

> The issue is where we account these pinned pages, where accounting is
> necessary such that a user cannot lock an arbitrary number of pages
> into RAM to generate a DoS attack.  

It is worth pointing out that preventing a DOS attack doesn't actually
work because a *task* limit is trivially bypassed by just spawning
more tasks. So, as a security feature, this is already very
questionable.

What we've done here is make the security feature work to actually
prevent DOS attacks, which then gives you this problem:

> This obviously has implications.  AFAICT, any management tool that
> doesn't instantiate assigned device VMs under separate users are
> essentially untenable.

Because now that the security feature works properly it detects the
DOS created by spawning multiple tasks :(

Somehow I was under the impression there was not user sharing in the
common cases, but I guess I don't know that for sure.

> > So, I still like 2 because it yields the smallest next step before we
> > can bring all the parallel work onto the list, and it makes testing
> > and converting non-qemu stuff easier even going forward.
> 
> If a vfio compatible interface isn't transparently compatible, then I
> have a hard time understanding its value.  Please correct my above
> description and implications, but I suspect these are not just
> theoretical ABI compat issues.  Thanks,

Because it is just fine for everything that doesn't use the ulimit
feature, which is still a lot of use cases!

Remember, at this point we are not replacing /dev/vfio/vfio, this is
just providing the general compat in a form that has to be opted
into. I think if you open the /dev/iommu device node then you should
get secured accounting.

If /dev/vfio/vfio is provided by iommufd it may well have to trigger a
different ulimit tracking - if that is the only sticking point it
seems minor and should be addressed in some later series that adds
/dev/vfio/vfio support to iommufd..

Jason