Messages in this thread | | | Subject | Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation | From | Christian König <> | Date | Fri, 14 Jan 2022 08:16:30 +0100 |
| |
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? >> 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. > 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.
Regards, Christian.
> > thanks > -john
| |