Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation

[Guangming.Cao <guangming.cao@mediatek.com>]

On Fri, 2022-01-14 at 17:17 -0800, John Stultz wrote:
> On Fri, Jan 14, 2022 at 4:04 AM Guangming.Cao
> <guangming.cao@mediatek.com> wrote:
> > 
> > On Fri, 2022-01-14 at 08:16 +0100, Christian König wrote:
> > > Am 14.01.22 um 00:26 schrieb John Stultz:
> > > > On Thu, Jan 13, 2022 at 5:05 AM Christian König
> > > > <christian.koenig@amd.com> wrote:
> > > > > Am 13.01.22 um 14:00 schrieb Ruhl, Michael J:
> > > > > > > -----Original Message-----
> > > > > > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org>
> > > > > > > On
> > > > > > > Behalf Of
> > > > > > > Ruhl, Michael J
> > > > > > > > -----Original Message-----
> > > > > > > > From: dri-devel <
> > > > > > > > dri-devel-bounces@lists.freedesktop.org>
> > > > > > > > On Behalf Of
> > > > > > > > guangming.cao@mediatek.com
> > > > > > > > +   /*
> > > > > > > > +    * Invalid size check. The "len" should be less
> > > > > > > > than
> > > > > > > > totalram.
> > > > > > > > +    *
> > > > > > > > +    * Without this check, once the invalid size
> > > > > > > > allocation
> > > > > > > > runs on a process
> > > > > > > > that
> > > > > > > > +    * can't be killed by OOM flow(such as "gralloc" on
> > > > > > > > Android devices), it
> > > > > > > > will
> > > > > > > > +    * cause a kernel exception, and to make matters
> > > > > > > > worse,
> > > > > > > > we can't find
> > > > > > > > who are using
> > > > > > > > +    * so many memory with "dma_buf_debug_show" since
> > > > > > > > the
> > > > > > > > relevant
> > > > > > > > dma-buf hasn't exported.
> > > > > > > > +    */
> > > > > > > > +   if (len >> PAGE_SHIFT > totalram_pages())
> > > > > > > 
> > > > > > > If your "heap" is from cma, is this still a valid check?
> > > > > > 
> > > > > > And thinking a bit further, if I create a heap from
> > > > > > something
> > > > > > else (say device memory),
> > > > > > you will need to be able to figure out the maximum
> > > > > > allowable
> > > > > > check for the specific
> > > > > > heap.
> > > > > > 
> > > > > > Maybe the heap needs a callback for max size?
> > 
> > Yes, I agree with this solution.
> > If dma-heap framework support this via adding a callback to support
> > it,
> > seems it's more clear than adding a limitation in dma-heap
> > framework
> > since each heap maybe has different limitation.
> > If you prefer adding callback, I can update this patch and add
> > totalram
> > limitation to system dma-heap.
> 
> If the max value is per-heap, why not enforce that value in the
> per-heap allocation function?
> 
> Moving the check to the heap alloc to me seems simpler to me than
> adding complexity to the infrastructure to add a heap max_size
> callback. Is there some other use for the callback that you envision?
> 
> thanks
> -john

Thanks for your comment.

If you think max the value is per-heap, why not add an optional
callback for dma-heap to solve this issue(prevent consuming too much
time for a doomed to fail allocation), if the dma-heap doesn't have a
special size check, just use the default value(totalram) in dma-heap 
framework to do the size check.

Yes, for linux dma-heaps, only system-heap needs it, so adding it in
system heap is the simplest. However, there are many vendor dma-heaps
like system-heap which won't be uploaded to linux codebase, and maybe
have same limitation, all these heaps need to add the same limitation.
I just think it's boring. However, If you think discussing these absent
cases based on current linux code is meaningless, I also agree to it.

So, to summarize, if you still think adding it in system_heap.c is
better, I also agree and I will update the patch to add it in
system_heap.c

Thanks~
Guangming

> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek