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 <[email protected]> wrote: > > > From: John Stultz <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: T.J. Mercier <[email protected]> > > Signed-off-by: Yong Wu <[email protected]> > > [Yong: Just add comment for "minor" and "refcount"] > > Signed-off-by: Yunfei Dong <[email protected]> > > [Yunfei: Change reviewer's comments] > > Signed-off-by: Florent Tomasin <[email protected]> > > [Florent: Rebase] > > Signed-off-by: Ketil Johnsen <[email protected]> > > [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/[email protected] 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+j3d...@mail.gmail.com/ Maxime
signature.asc
Description: PGP signature
