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

[Guangming.Cao]

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 at amd.com> wrote:
> > > Am 13.01.22 um 14:00 schrieb Ruhl, Michael J:
> > > > > -----Original Message-----
> > > > > From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On
> > > > > Behalf Of
> > > > > Ruhl, Michael J
> > > > > > -----Original Message-----
> > > > > > From: dri-devel <dri-devel-bounces at lists.freedesktop.org>
> > > > > > On Behalf Of
> > > > > > guangming.cao at 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.

Thanks!
Guangming
> > > 
> > > Well we currently maintain a separate allocator and don't use
> > > dma-heap,
> > > but yes we have systems with 16GiB device and only 8GiB system
> > > memory so
> > > that check here is certainly not correct.
> > 
> > Good point.
> > 
> > > In general I would rather let the system run into -ENOMEM or
> > > -EINVAL
> > > from the allocator instead.

For system dma-heap, it doesn't know how memory is avaliable when
allocating memory, so, use totalram_pages() just to prevent cases which
will cause oom definitely.

Just like PAGE align, this check is can be used for all heaps since
there is no dma-heap can alloc memory larger than totalram. Futhermore,
if vendors implement a variety of dma-heap like system heap for special
usages, seems need to add this check to each dma-heap, and I think this
is unnecessary.
If the dma-heap has it's own special limitations for size, and add it
into heap implementation is good.

Thanks!
Guangming
> > 
> > Probably the simpler solution is to push the allocation check to
> > the
> > heap driver, rather than doing it at the top level here.
> > 
> > For CMA or other contiguous heaps, letting the allocator fail is
> > fast
> > enough. For noncontiguous buffers, like the system heap, the
> > allocation can burn a lot of time and consume a lot of memory
> > (causing
> > other trouble) before a large allocation might naturally fail.
> 
> Yeah, letting a alloc_page() loop run for a while is usually not nice
> at 
> all :)
> 
> You can still do a sanity check here, e.g. the size should never
> have 
> the most significant bit set for example.
> 
Yes, this is a good solution. But if this a positive value, larger than
totalram, it can also pass this check, and cause OOM after some time.

From dicussion above, seems finding a proper solution that can judge
the size is valid or not for each dma-heap is more important.

Thanks!
Guangming

> Regards,
> Christian.
> 
> > 
> > thanks
> > -john
> 
>