Re: [PATCH RFC v2 00/13] IOMMUFD Generic interface
On 9/21/2022 2:44 PM, Jason Gunthorpe wrote:
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.
"unhappy" barely scratches the surface of my feelings!
Live update is a great feature that solves a real problem, and lots of
people have spent lots of time providing thorough feedback on the qemu
patches I have submitted. We *will* cross the finish line. In the
mean time, I maintain a patched qemu for use in my company, and I have
heard from others who do the same.
If Steve wants to keep it then someone needs to fix the deadlock in
the vfio implementation before any userspace starts to appear.
The only VFIO_DMA_UNMAP_FLAG_VADDR issue I am aware of is broken pinned accounting
across exec, which can result in mm->locked_vm becoming negative. I have several
fixes, but none result in limits being reached at exactly the same time as before --
the same general issue being discussed for iommufd. I am still thinking about it.
I am not aware of a deadlock problem. Please elaborate or point me to an
email thread.
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.
memfd support alone is not sufficient. Live update also supports guest ram
backed by named shared memory.
- Steve
> 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