From: Boris Brezillon <boris.brezillon@collabora•com>
To: Maxime Ripard <mripard@kernel•org>
Cc: "Ketil Johnsen" <ketil.johnsen@arm•com>,
"David Airlie" <airlied@gmail•com>,
"Simona Vetter" <simona@ffwll•ch>,
"Maarten Lankhorst" <maarten.lankhorst@linux•intel.com>,
"Thomas Zimmermann" <tzimmermann@suse•de>,
"Jonathan Corbet" <corbet@lwn•net>,
"Shuah Khan" <skhan@linuxfoundation•org>,
"Sumit Semwal" <sumit.semwal@linaro•org>,
"Benjamin Gaignard" <benjamin.gaignard@collabora•com>,
"Brian Starkey" <Brian.Starkey@arm•com>,
"John Stultz" <jstultz@google•com>,
"T.J. Mercier" <tjmercier@google•com>,
"Christian König" <christian.koenig@amd•com>,
"Steven Price" <steven.price@arm•com>,
"Liviu Dudau" <liviu.dudau@arm•com>,
"Daniel Almeida" <daniel.almeida@collabora•com>,
"Alice Ryhl" <aliceryhl@google•com>,
"Matthias Brugger" <matthias.bgg@gmail•com>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora•com>,
dri-devel@lists•freedesktop.org, linux-doc@vger•kernel.org,
linux-kernel@vger•kernel.org, linux-media@vger•kernel.org,
linaro-mm-sig@lists•linaro.org,
linux-arm-kernel@lists•infradead.org,
linux-mediatek@lists•infradead.org,
"Yong Wu" <yong.wu@mediatek•com>,
"Yunfei Dong" <yunfei.dong@mediatek•com>,
"Florent Tomasin" <florent.tomasin@arm•com>
Subject: Re: [PATCH 1/8] dma-heap: Add proper kref handling on dma-buf heaps
Date: Tue, 5 May 2026 18:40:21 +0200 [thread overview]
Message-ID: <20260505184021.3676f9af@fedora> (raw)
In-Reply-To: <20260505-spaniel-of-scientific-warranty-ca075e@houat>
On Tue, 5 May 2026 17:39:13 +0200
Maxime Ripard <mripard@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@arm•com> wrote:
> >
> > > From: John Stultz <jstultz@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@google•com>
> > > Signed-off-by: T.J. Mercier <tjmercier@google•com>
> > > Signed-off-by: Yong Wu <yong.wu@mediatek•com>
> > > [Yong: Just add comment for "minor" and "refcount"]
> > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek•com>
> > > [Yunfei: Change reviewer's comments]
> > > Signed-off-by: Florent Tomasin <florent.tomasin@arm•com>
> > > [Florent: Rebase]
> > > Signed-off-by: Ketil Johnsen <ketil.johnsen@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...
next prev parent reply other threads:[~2026-05-05 16:40 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-05 14:05 [PATCH 0/8] drm/panthor: Protected mode support for Mali CSF GPUs Ketil Johnsen
2026-05-05 14:05 ` [PATCH 1/8] dma-heap: Add proper kref handling on dma-buf heaps Ketil Johnsen
2026-05-05 15:20 ` Boris Brezillon
2026-05-05 15:39 ` Maxime Ripard
2026-05-05 16:40 ` Boris Brezillon [this message]
2026-05-07 15:33 ` Maxime Ripard
2026-05-05 14:05 ` [PATCH 2/8] dma-heap: Provide accessors so that in-kernel drivers can allocate dmabufs from specific heaps Ketil Johnsen
2026-05-05 15:45 ` Boris Brezillon
2026-05-05 14:05 ` [PATCH 3/8] drm/panthor: De-duplicate FW memory section sync Ketil Johnsen
2026-05-05 15:47 ` Boris Brezillon
2026-05-12 13:37 ` Liviu Dudau
2026-05-05 14:05 ` [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor Ketil Johnsen
2026-05-05 16:15 ` Boris Brezillon
2026-05-07 9:02 ` Marcin Ślusarz
2026-05-07 11:53 ` Boris Brezillon
2026-05-12 13:47 ` Liviu Dudau
2026-05-12 14:11 ` Boris Brezillon
2026-05-12 15:38 ` Liviu Dudau
2026-05-13 19:31 ` Chia-I Wu
2026-05-18 7:16 ` Boris Brezillon
2026-05-19 0:36 ` Chia-I Wu
2026-05-19 7:39 ` Boris Brezillon
2026-05-19 8:49 ` Ketil Johnsen
2026-05-19 17:07 ` Chia-I Wu
2026-05-19 17:29 ` Boris Brezillon
2026-05-19 9:52 ` [Linaro-mm-sig] " Maxime Ripard
2026-05-19 11:37 ` Boris Brezillon
2026-05-06 10:08 ` Maxime Ripard
2026-05-06 10:50 ` Boris Brezillon
2026-05-06 13:12 ` Maxime Ripard
2026-05-06 15:05 ` Boris Brezillon
2026-05-07 13:39 ` Thierry Reding
2026-05-06 12:43 ` Nicolas Frattaroli
2026-05-06 13:31 ` Maxime Ripard
2026-05-06 12:28 ` Nicolas Frattaroli
2026-05-05 14:05 ` [PATCH 5/8] drm/panthor: Minor scheduler refactoring Ketil Johnsen
2026-05-05 16:19 ` Boris Brezillon
2026-05-06 10:33 ` Boris Brezillon
2026-05-05 14:05 ` [PATCH 6/8] drm/panthor: Explicit expansion of locked VM region Ketil Johnsen
2026-05-05 16:32 ` Boris Brezillon
2026-05-06 15:14 ` Nicolas Frattaroli
2026-05-07 14:54 ` Nicolas Frattaroli
2026-05-05 14:05 ` [PATCH 7/8] drm/panthor: Add support for entering and exiting protected mode Ketil Johnsen
2026-05-05 17:11 ` Boris Brezillon
2026-05-06 8:51 ` Boris Brezillon
2026-05-05 14:05 ` [PATCH 8/8] drm/panthor: Expose protected rendering features Ketil Johnsen
2026-05-06 9:14 ` Boris Brezillon
2026-05-07 8:47 ` Marcin Ślusarz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260505184021.3676f9af@fedora \
--to=boris.brezillon@collabora$(echo .)com \
--cc=Brian.Starkey@arm$(echo .)com \
--cc=airlied@gmail$(echo .)com \
--cc=aliceryhl@google$(echo .)com \
--cc=angelogioacchino.delregno@collabora$(echo .)com \
--cc=benjamin.gaignard@collabora$(echo .)com \
--cc=christian.koenig@amd$(echo .)com \
--cc=corbet@lwn$(echo .)net \
--cc=daniel.almeida@collabora$(echo .)com \
--cc=dri-devel@lists$(echo .)freedesktop.org \
--cc=florent.tomasin@arm$(echo .)com \
--cc=jstultz@google$(echo .)com \
--cc=ketil.johnsen@arm$(echo .)com \
--cc=linaro-mm-sig@lists$(echo .)linaro.org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-doc@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-media@vger$(echo .)kernel.org \
--cc=linux-mediatek@lists$(echo .)infradead.org \
--cc=liviu.dudau@arm$(echo .)com \
--cc=maarten.lankhorst@linux$(echo .)intel.com \
--cc=matthias.bgg@gmail$(echo .)com \
--cc=mripard@kernel$(echo .)org \
--cc=simona@ffwll$(echo .)ch \
--cc=skhan@linuxfoundation$(echo .)org \
--cc=steven.price@arm$(echo .)com \
--cc=sumit.semwal@linaro$(echo .)org \
--cc=tjmercier@google$(echo .)com \
--cc=tzimmermann@suse$(echo .)de \
--cc=yong.wu@mediatek$(echo .)com \
--cc=yunfei.dong@mediatek$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox