Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Mar 07, 2025 at 12:57:51PM -0400, Jason Gunthorpe wrote:
> 
> Why are you explaining very simple concepts as though I do not
> understand how RCU or devm works?
> 
> I asked you what you intend to protect with the critical region.

When you ask what the critical region protects, I think that it protects the
resource pointer from changing.

I did not read it as "what's meant to be within the critical region".

> 
> I belive you intend to wrapper every memcpy/etc of the allocated
> coherent memory with a RCU critical section, correct?
> 
> Meaning something like:
> 
>   mem.ptr = dma_alloc_coherent(&handle)
>   make_hw_do_dma(handle)
> 
>   start RCU critical section on mem:
>       copy_to_user(mem.ptr) // Sleeps! Can't do it!
>   dma_free_coherent(mem, handle)
> 
> Right?

Yes, that would indeed be a problem. Thanks for pointing it out.

While we could do an SRCU variant, provide separate try_access() methods, etc.,
I think we should do something more efficient:

There is no reason to revoke the *whole* CoherentAllocation object, but only the
the parts that are critical to actually cleanup on driver unbind, i.e. iommu
mappings, etc.

The actual memory allocation itself is not an issue in terms of living beyond
driver unbind and hence doesn't need to be revoked.

With this, you would not need any critical section to access the
CoherentAllocation's memory from the driver.

> Further, if the critical section ever fails to obtain mem.ptr the
> above code is *BUGGY* because it has left a HW DMA running, UAF'd the
> now free'd buffer *and the driver author cannot fix it*.

I don't think that'd be the case in a Rust driver, your example is in C, and
hence doesn't do the Rust style error and cleanup handling that the
corresponding Rust code would do.

But as mentioned above, putting the whole CoherentAllocation in a Devres
container seems wrong anyways. We need to do it at a finer granularity.