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@xxxxxxx> wrote: > > > Am 13.01.22 um 14:00 schrieb Ruhl, Michael J: > > > > > -----Original Message----- > > > > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On > > > > > Behalf Of > > > > > Ruhl, Michael J > > > > > > -----Original Message----- > > > > > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> > > > > > > On Behalf Of > > > > > > guangming.cao@xxxxxxxxxxxx > > > > > > + /* > > > > > > + * 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 > >
