[PATCH 1/8] dma-heap: Add proper kref handling on dma-buf heaps

[Maxime Ripard]

On Tue, May 05, 2026 at 06:40:21PM +0200, Boris Brezillon wrote:
> On Tue, 5 May 2026 17:39:13 +0200
> Maxime Ripard <mripard at kernel.org> wrote:
> 
> > Hi Boris,
> > 
> > On Tue, May 05, 2026 at 05:20:48PM +0200, Boris Brezillon wrote:
> > > Hi Ketil,
> > > 
> > > On Tue,  5 May 2026 16:05:07 +0200
> > > Ketil Johnsen <ketil.johnsen at arm.com> wrote:
> > >   
> > > > From: John Stultz <jstultz at google.com>
> > > > 
> > > > Add proper reference counting on the dma_heap structure. While
> > > > existing heaps are built-in, we may eventually have heaps loaded
> > > > from modules, and we'll need to be able to properly handle the
> > > > references to the heaps  
> > > 
> > > It's weird that this "heap as module" thing is mentioned here, but
> > > actual robustness to make this safe is not added in the commit or any
> > > of the following ones.
> > >   
> > > > 
> > > > Signed-off-by: John Stultz <jstultz at google.com>
> > > > Signed-off-by: T.J. Mercier <tjmercier at google.com>
> > > > Signed-off-by: Yong Wu <yong.wu at mediatek.com>
> > > > [Yong: Just add comment for "minor" and "refcount"]
> > > > Signed-off-by: Yunfei Dong <yunfei.dong at mediatek.com>
> > > > [Yunfei: Change reviewer's comments]
> > > > Signed-off-by: Florent Tomasin <florent.tomasin at arm.com>
> > > > [Florent: Rebase]
> > > > Signed-off-by: Ketil Johnsen <ketil.johnsen at arm.com>
> > > > [Ketil: Rebase]
> > > > ---
> > > >  drivers/dma-buf/dma-heap.c | 29 +++++++++++++++++++++++++++++
> > > >  include/linux/dma-heap.h   |  2 ++
> > > >  2 files changed, 31 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > > > index ac5f8685a6494..9fd365ddbd517 100644
> > > > --- a/drivers/dma-buf/dma-heap.c
> > > > +++ b/drivers/dma-buf/dma-heap.c
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/dma-heap.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/export.h>
> > > > +#include <linux/kref.h>
> > > >  #include <linux/list.h>
> > > >  #include <linux/nospec.h>
> > > >  #include <linux/syscalls.h>
> > > > @@ -31,6 +32,7 @@
> > > >   * @heap_devt:		heap device node
> > > >   * @list:		list head connecting to list of heaps
> > > >   * @heap_cdev:		heap char device
> > > > + * @refcount:		reference counter for this heap device
> > > >   *
> > > >   * Represents a heap of memory from which buffers can be made.
> > > >   */
> > > > @@ -41,6 +43,7 @@ struct dma_heap {
> > > >  	dev_t heap_devt;
> > > >  	struct list_head list;
> > > >  	struct cdev heap_cdev;
> > > > +	struct kref refcount;
> > > >  };
> > > >  
> > > >  static LIST_HEAD(heap_list);
> > > > @@ -248,6 +251,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> > > >  	if (!heap)
> > > >  		return ERR_PTR(-ENOMEM);
> > > >  
> > > > +	kref_init(&heap->refcount);
> > > >  	heap->name = exp_info->name;
> > > >  	heap->ops = exp_info->ops;
> > > >  	heap->priv = exp_info->priv;
> > > > @@ -313,6 +317,31 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> > > >  }
> > > >  EXPORT_SYMBOL_NS_GPL(dma_heap_add, "DMA_BUF_HEAP");
> > > >  
> > > > +static void dma_heap_release(struct kref *ref)
> > > > +{
> > > > +	struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
> > > > +	unsigned int minor = MINOR(heap->heap_devt);
> > > > +
> > > > +	mutex_lock(&heap_list_lock);
> > > > +	list_del(&heap->list);
> > > > +	mutex_unlock(&heap_list_lock);
> > > > +
> > > > +	device_destroy(dma_heap_class, heap->heap_devt);
> > > > +	cdev_del(&heap->heap_cdev);
> > > > +	xa_erase(&dma_heap_minors, minor);
> > > > +
> > > > +	kfree(heap);  
> > > 
> > > That's actually problematic, because cdev_del() doesn't guarantee that
> > > all opened FDs have been closed [1], it just guarantees that no new ones
> > > can materialize. In order to make that safe, we'd need a
> > > 
> > > 1. kref_get_unless_zero() in dma_heap_open(), with proper locking around
> > >    the xa_load() to protect against the heap removal that's happening
> > >    here
> > > 2. a dma_heap_put() in a new dma_heap_close() implementation
> > > 3. a guarantee that heap implementations won't go away until the last
> > >    ref is dropped, which means ops and all the data needed for this heap
> > >    to satisfy ioctl()s (and more generally every passed at
> > >    dma_heap_add() time) have to stay valid until the last ref is
> > >    dropped. Alternatively, we could restrict this only to in-flight
> > >    ioctl()s, and have the ops replaced by some dummy ops using RCU or a
> > >    rwlock. But I guess live dmabufs allocated on this heap have to
> > >    retain the heap and its implementation anyway.
> > > 
> > > For record, #3 is already not satisfied by the current tee_heap
> > > implementation (tee_dma_heap objects can vanish before the dma_heap
> > > object is gone). The other implementations seem to be fine because they
> > > are statically linked, and they either have exp_info.priv set to NULL,
> > > or something that's never released.  
> > 
> > That statement won't hold for long, see:
> > https://lore.kernel.org/r/20260427-dma-buf-heaps-as-modules-v5-0-b6f5678feefc@kernel.org
> > 
> > However, all upstream heaps can be loaded as module, but not unloaded.
> > So once you get a reference to it, you can assume it will live forever.
> > That's why we didn't merge that patch before, even though it was discussed:
> > 
> > https://lore.kernel.org/all/CANDhNCqk9Uk4aXHhUsL4hR1GHNmWZnH3C9Np-A02wdi+J3D7tA@mail.gmail.com/
> 
> Hm, not too sure that makes the tee_heap implementation sane WRT
> tee_heap removal though, unless we have a guarantee that
> tee_device_unregister() will never be called...

I missed that part. You're totally right then :)

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20260507/a8c103f4/attachment.sig>