linux-kernel - Re: [PATCH v4 3/7] dma-buf: heaps: restricted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANDhNCqieBaH-Wi=vy3NSKTpwHcWef6qMOFi-7sgdGiDW7JtwA@mail.gmail.com>
Date: Fri, 12 Jan 2024 15:51:25 -0800
From: John Stultz <jstultz@...gle.com>
To: Jeffrey Kardatzke <jkardatzke@...gle.com>
Cc: Yong Wu <yong.wu@...iatek.com>, Rob Herring <robh+dt@...nel.org>, 
	Matthias Brugger <matthias.bgg@...il.com>, christian.koenig@....com, 
	Sumit Semwal <sumit.semwal@...aro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Benjamin Gaignard <benjamin.gaignard@...labora.com>, Brian Starkey <Brian.Starkey@....com>, 
	tjmercier@...gle.com, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	Robin Murphy <robin.murphy@....com>, Vijayanand Jitta <quic_vjitta@...cinc.com>, 
	Joakim Bech <joakim.bech@...aro.org>, Pavel Machek <pavel@....cz>, Simon Ser <contact@...rsion.fr>, 
	Pekka Paalanen <ppaalanen@...il.com>, jianjiao.zeng@...iatek.com, kuohong.wang@...iatek.com, 
	youlin.pei@...iatek.com
Subject: Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops

On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke <jkardatzke@...gle.com> wrote:
> On Fri, Jan 12, 2024 at 2:52 PM John Stultz <jstultz@...gle.com> wrote:
> > On Fri, Jan 12, 2024 at 1:21 AM Yong Wu <yong.wu@...iatek.com> wrote:
> > > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
> > > index 443028f6ba3b..ddeaf9805708 100644
> > > --- a/drivers/dma-buf/heaps/restricted_heap.h
> > > +++ b/drivers/dma-buf/heaps/restricted_heap.h
> > > @@ -15,6 +15,18 @@ struct restricted_buffer {
> > >
> > >  struct restricted_heap {
> > >         const char              *name;
> > > +
> > > +       const struct restricted_heap_ops *ops;
> > > +};
> > > +
> > > +struct restricted_heap_ops {
> > > +       int     (*heap_init)(struct restricted_heap *heap);
> > > +
> > > +       int     (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf);
> > > +       void    (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf);
> > > +
> > > +       int     (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
> > > +       void    (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
> > >  };
> > >
> > >  int restricted_heap_add(struct restricted_heap *rstrd_heap);
> >
> > So, I'm a little worried here, because you're basically turning the
> > restricted_heap dma-buf heap driver into a framework itself.
> > Where this patch is creating a subdriver framework.
> >
> > Part of my hesitancy, is you're introducing this under the dma-buf
> > heaps. For things like CMA, that's more of a core subsystem that has
> > multiple users, and exporting cma buffers via dmabuf heaps is just an
> > additional interface.  What I like about that is the core kernel has
> > to define the semantics for the memory type and then the dmabuf heap
> > is just exporting that well understood type of buffer.
> >
> > But with these restricted buffers, I'm not sure there's yet a well
> > understood set of semantics nor a central abstraction for that which
> > other drivers use directly.
> >
> > I know part of this effort here is to start to centralize all these
> > vendor specific restricted buffer implementations, which I think is
> > great, but I just worry that in doing it in the dmabuf heap interface,
> > it is a bit too user-facing. The likelihood of encoding a particular
> > semantic to the singular "restricted_heap" name seems high.
>
> In patch #5 it has the actual driver implementation for the MTK heap
> that includes the heap name of "restricted_mtk_cm", so there shouldn't
> be a heap named "restricted_heap" that's actually getting exposed. The

Ah, I appreciate that clarification! Indeed, you're right the name is
passed through. Apologies for missing that detail.

> restricted_heap code is a framework, and not a driver itself.  Unless
> I'm missing something in this patchset...but that's the way it's
> *supposed* to be.

So I guess I'm not sure I understand the benefit of the extra
indirection. What then does the restricted_heap.c logic itself
provide?
The dmabuf heaps framework already provides a way to add heap implementations.

thanks
-john