Am 21.06.24 um 00:02 schrieb Xu Yilun:

On Thu, Jan 16, 2025 at 04:13:13PM +0100, Christian König wrote:

...

Am 15.01.25 um 18:09 schrieb Jason Gunthorpe:

On Wed, Jan 15, 2025 at 05:34:23PM +0100, Christian König wrote:

 Granted, let me try to improve this.
 Here is a real world example of one of the issues we ran into and why
 CPU mappings of importers are redirected to the exporter.
 We have a good bunch of different exporters who track the CPU mappings
 of their backing store using address_space objects in one way or
 another and then uses unmap_mapping_range() to invalidate those CPU
 mappings.
 But when importers get the PFNs of the backing store they can look
 behind the curtain and directly insert this PFN into the CPU page
 tables.
 We had literally tons of cases like this where drivers developers cause
 access after free issues because the importer created a CPU mappings on
 their own without the exporter knowing about it.
 This is just one example of what we ran into. Additional to that
 basically the whole synchronization between drivers was overhauled as
 well because we found that we can't trust importers to always do the
 right thing.

But this, fundamentally, is importers creating attachments and then *ignoring the lifetime rules of DMABUF*. If you created an attachment, got a move and *ignored the move* because you put the PFN in your own VMA, then you are not following the attachment lifetime rules!

Move notify is solely for informing the importer that they need to
re-fresh their DMA mappings and eventually block for ongoing DMA to end.

This semantics doesn't work well for CPU mappings because you need to hold
the reservation lock to make sure that the information stay valid and you
can't hold a lock while returning from a page fault.

Dealing with CPU mapping and resource invalidation is a little hard, but is resolvable, by using other types of locks. And I guess for now dma-buf exporters should always handle this CPU mapping VS. invalidation contention if they support mmap().

It is resolvable so with some invalidation notify, a decent importers could also handle the contention well.

That doesn't work like this.

See page tables updates under DMA-buf works by using the same locking approach for both the validation and invalidation side. In other words we hold the same lock while inserting and removing entries into/from the page tables.

That this here should be an unlocked API means that can only use it with pre-allocated and hard pinned memory without any chance to invalidate it while running. Otherwise you can never be sure of the validity of the address information you got from the exporter.

IIUC now the only concern is importer device drivers are easier to do something wrong, so move CPU mapping things to exporter. But most of the exporters are also device drivers, why they are smarter?

Exporters always use their invalidation code path no matter if they are exporting their buffers for other to use or if they are stand alone.

If you do the invalidation on the importer side you always need both exporter and importer around to test it.

Additional to that we have much more importers than exporters. E.g. a lot of simple drivers only import DMA-heap buffers and never exports anything.

And there are increasing mapping needs, today exporters help handle CPU primary mapping, tomorrow should they also help on all other mappings? Clearly it is not feasible. So maybe conditionally give trust to some importers.

Why should that be necessary? Exporters *must* know what somebody does with their buffers.

If you have an use case the exporter doesn't support in their mapping operation then that use case most likely doesn't work in the first place.

For example direct I/O is enabled/disabled by exporters on their CPU mappings based on if that works correctly for them. And importer simply doesn't know if they should use vm_insert_pfn() or vm_insert_page().

We could of course implement that logic into each importer to chose between the different approaches, but than each importer gains logic it only exercises with a specific exporter. And that doesn't seem to be a good idea at all.

Regards, Christian.

Thanks, Yilun