Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

On 8/5/26 12:59, Pavel Begunkov wrote:
> On 8/5/26 09:27, Christian König wrote:

>>> +
>>> +    dma_resv_lock(dmabuf->resv, NULL);
>>> +    ctx->dev_ops->unmap(ctx, map);
>>> +    dma_resv_unlock(dmabuf->resv);
>>> +
>>> +    dma_fence_put(&fence->base);
>>
>> You should probably set map->fence to NULL after that.
> 
> The map is freed two lines below, but I can add it as
> a defensive measure.

In that case it's ok, I've just haven't seen the kfree(map) below.

> ...
>>> +    ret = dma_resv_reserve_fences(dmabuf->resv, 1);
>>> +    if (WARN_ON_ONCE(ret)) {
>>> +        struct dma_fence *fence = &map->fence->base;
>>> +
>>> +        dma_fence_get(fence);
>>> +        percpu_ref_kill(&map->refs);
>>> +        dma_fence_wait(fence, false);
>>> +        dma_fence_put(fence);
>>> +        return;
>>> +    }
>>> +
>>> +    dma_resv_add_fence(dmabuf->resv, &map->fence->base,
>>> +               DMA_RESV_USAGE_KERNEL);
>>
>> That sequence is clearly incorrect!
>>
>> The fence must be created after dma_resv_reserve_fences(), otherwise you definately have an illegal memory operation here.
> 
> I'm not sure what you mean, can you elaborate? I only cared about
> pre-allocating it to avoid allocations here. We add / signal the fence
> only once, no reuse. The map is going to be killed here, and if we
> create a new map, it'll have its own fence.
> 
> I can move the dma_fence_init() call here if that makes a difference?

Yeah that is a good start, but you might need a bit more.

Here is a summary of the usual procedure you need to follow when implementing a dma_fence backend:

1. Allocate your operation object, in this case here it's your mapping I think.
2. Prepare your operation, including all memory allocations.
3. Call dma_resv_reserve_fences() to reserve a fence slot.
4. Allocate and init your dma_fence object.

After this step no memory allocation is allowed any more until your dma_fence object signals.

The only exception is optional logging or crash dumping using GFP_NOWAIT (can fail trivially!) or minimal allocations using GFP_ATOMIC if you absolutely have to.

5. dma_resv_add_fence() to publish the fence.
6. dma_resv_unlock().

Having a dma_fence is certainly nice to have, but the tricky part is that memory allocations using GFP_KERNEL (or GFP_IO, GFP_FS etc...) can cycle back and wait for your dma_fence to signal which essentially can cause a deadlock very deeply inside memory management.

Since those deadlocks happen only on memory contention situations they are usually just hard to reproduce but still totally break your neck if you manage to mess this up. So that needs to be super carefully implemented.

Regards,
Christian.