Re: [PATCH v12 1/5] dma-buf: Add dma-buf heaps framework

[John Stultz]

On Mon, Oct 21, 2019 at 2:36 AM Brian Starkey <Brian.Starkey@xxxxxxx> wrote:
> On Fri, Oct 18, 2019 at 11:26:52AM -0700, John Stultz wrote:
> > On Fri, Oct 18, 2019 at 4:18 AM Brian Starkey <Brian.Starkey@xxxxxxx> wrote:
> > > On Fri, Oct 18, 2019 at 05:23:19AM +0000, John Stultz wrote:
> > >
> > > As in v3:
> > >
> > >  * Avoid EXPORT_SYMBOL until we finalize modules (suggested by
> > >    Brian)
> >
> > Heh. I guess it has been awhile.  :)
> >
> > > Did something change in that regard? I still think letting modules
> > > register heaps without a way to remove them is a recipe for issues.
> >
> > So yea, in recent months, work around Android with their GKI effort
> > has made it necessary for ION heaps to be loadable from modules. I had
> > some patches in WIP tree to enable this, and in the rework I did
> > yesterday for the CMA module trivially collided with parts, and
> > forgetting the discussion back in v3, I figured I'd just fold those
> > bits in before I resubmitted for v12.
>
> Ah yes, I can see that would be needed.
>
> >
> > If it's an issue, I can pull it out, but I'm going to be submitting
> > module enablement for review as soon as the core bits are queued, as
> > its going to be important to support for Android to switch to this
> > from ION.
> >
>
> I don't know how to decide if it's an issue. My understanding is that
> if you rmmod something which has exported buffers, various Bad Things
> might happen; I believe including data leaks, corruption or crashing
> the kernel. There's probably plenty of scope for that with dma-buf
> exporters already, so it's not exactly "new" but it is a bit
> unfortunate.
>
> If "people" are OK with adding new code which has the same issue, then
> I'm not going to make any more of a fuss, because perfection is the
> enemy of progress. Perhaps an obvious comment pointing out the issue
> would be prudent, though - as a reminder to people that add heaps from
> their code (and wonder why there's no "dma_heap_remove" function).

Eh. If I need to respin anyway, I'll just remove the exports for now.

It's really just my fault for getting impatient and trying to squeeze
some extra changes in.

I'll then submit the module enablement patches separately.

Thanks again for the review, I really appreciate your sharp eye!
-john