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

Steven Sistare
2022-09-21T19:31:40Z

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