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

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

Posted by guangming.cao@mediatek.com 2 years, 8 months ago

From: Guangming <Guangming.Cao@mediatek.com>

Add a size check for allocation since the allocation size is
always less than the total DRAM size.

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.

To make OOM issue easier, maybe need dma-buf framework to dump the buffer size
under allocating in "dma_buf_debug_show".

Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
---
v3: 1. update patch, use right shift to replace division.
    2. update patch, add reason in code and commit message.
v2: 1. update size limitation as total_dram page size.
    2. update commit message
---
 drivers/dma-buf/dma-heap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 56bf5ad01ad5..1fd382712584 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -55,6 +55,16 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
 	struct dma_buf *dmabuf;
 	int fd;
 
+	/*
+	 * 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())
+		return -EINVAL;
 	/*
 	 * Allocations from all heaps have to begin
 	 * and end on page boundaries.
-- 
2.17.1

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

Posted by Ruhl, Michael J 2 years, 8 months ago

>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>guangming.cao@mediatek.com
>Sent: Thursday, January 13, 2022 7:34 AM
>To: sumit.semwal@linaro.org
>Cc: linux-arm-kernel@lists.infradead.org; mingyuan.ma@mediatek.com;
>Guangming <Guangming.Cao@mediatek.com>;
>wsd_upstream@mediatek.com; linux-kernel@vger.kernel.org; dri-
>devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org;
>yf.wang@mediatek.com; libo.kang@mediatek.com;
>benjamin.gaignard@linaro.org; bo.song@mediatek.com;
>matthias.bgg@gmail.com; linux-mediatek@lists.infradead.org;
>lmark@codeaurora.org; labbott@redhat.com; christian.koenig@amd.com;
>jianjiao.zeng@mediatek.com; linux-media@vger.kernel.org
>Subject: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
>
>From: Guangming <Guangming.Cao@mediatek.com>
>
>Add a size check for allocation since the allocation size is
>always less than the total DRAM size.
>
>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.
>
>To make OOM issue easier, maybe need dma-buf framework to dump the
>buffer size
>under allocating in "dma_buf_debug_show".
>
>Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
>Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
>---
>v3: 1. update patch, use right shift to replace division.
>    2. update patch, add reason in code and commit message.
>v2: 1. update size limitation as total_dram page size.
>    2. update commit message
>---
> drivers/dma-buf/dma-heap.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>index 56bf5ad01ad5..1fd382712584 100644
>--- a/drivers/dma-buf/dma-heap.c
>+++ b/drivers/dma-buf/dma-heap.c
>@@ -55,6 +55,16 @@ static int dma_heap_buffer_alloc(struct dma_heap
>*heap, size_t len,
> 	struct dma_buf *dmabuf;
> 	int fd;
>
>+	/*
>+	 * 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?

M

>+		return -EINVAL;
> 	/*
> 	 * Allocations from all heaps have to begin
> 	 * and end on page boundaries.
>--
>2.17.1

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

Posted by Ruhl, Michael J 2 years, 8 months ago

>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>Ruhl, Michael J
>Sent: Thursday, January 13, 2022 7:58 AM
>To: guangming.cao@mediatek.com; sumit.semwal@linaro.org
>Cc: jianjiao.zeng@mediatek.com; lmark@codeaurora.org;
>wsd_upstream@mediatek.com; christian.koenig@amd.com; linux-
>kernel@vger.kernel.org; dri-devel@lists.freedesktop.org;
>yf.wang@mediatek.com; linaro-mm-sig@lists.linaro.org; linux-
>mediatek@lists.infradead.org; libo.kang@mediatek.com;
>benjamin.gaignard@linaro.org; bo.song@mediatek.com;
>matthias.bgg@gmail.com; labbott@redhat.com;
>mingyuan.ma@mediatek.com; linux-arm-kernel@lists.infradead.org; linux-
>media@vger.kernel.org
>Subject: RE: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
>
>
>>-----Original Message-----
>>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>guangming.cao@mediatek.com
>>Sent: Thursday, January 13, 2022 7:34 AM
>>To: sumit.semwal@linaro.org
>>Cc: linux-arm-kernel@lists.infradead.org; mingyuan.ma@mediatek.com;
>>Guangming <Guangming.Cao@mediatek.com>;
>>wsd_upstream@mediatek.com; linux-kernel@vger.kernel.org; dri-
>>devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org;
>>yf.wang@mediatek.com; libo.kang@mediatek.com;
>>benjamin.gaignard@linaro.org; bo.song@mediatek.com;
>>matthias.bgg@gmail.com; linux-mediatek@lists.infradead.org;
>>lmark@codeaurora.org; labbott@redhat.com; christian.koenig@amd.com;
>>jianjiao.zeng@mediatek.com; linux-media@vger.kernel.org
>>Subject: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
>>
>>From: Guangming <Guangming.Cao@mediatek.com>
>>
>>Add a size check for allocation since the allocation size is
>>always less than the total DRAM size.
>>
>>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.
>>
>>To make OOM issue easier, maybe need dma-buf framework to dump the
>>buffer size
>>under allocating in "dma_buf_debug_show".
>>
>>Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
>>Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
>>---
>>v3: 1. update patch, use right shift to replace division.
>>    2. update patch, add reason in code and commit message.
>>v2: 1. update size limitation as total_dram page size.
>>    2. update commit message
>>---
>> drivers/dma-buf/dma-heap.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>>diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>>index 56bf5ad01ad5..1fd382712584 100644
>>--- a/drivers/dma-buf/dma-heap.c
>>+++ b/drivers/dma-buf/dma-heap.c
>>@@ -55,6 +55,16 @@ static int dma_heap_buffer_alloc(struct dma_heap
>>*heap, size_t len,
>> 	struct dma_buf *dmabuf;
>> 	int fd;
>>
>>+	/*
>>+	 * 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?

m
>M
>
>>+		return -EINVAL;
>> 	/*
>> 	 * Allocations from all heaps have to begin
>> 	 * and end on page boundaries.
>>--
>>2.17.1

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

Posted by Christian König 2 years, 8 months ago

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
>> Sent: Thursday, January 13, 2022 7:58 AM
>> To: guangming.cao@mediatek.com; sumit.semwal@linaro.org
>> Cc: jianjiao.zeng@mediatek.com; lmark@codeaurora.org;
>> wsd_upstream@mediatek.com; christian.koenig@amd.com; linux-
>> kernel@vger.kernel.org; dri-devel@lists.freedesktop.org;
>> yf.wang@mediatek.com; linaro-mm-sig@lists.linaro.org; linux-
>> mediatek@lists.infradead.org; libo.kang@mediatek.com;
>> benjamin.gaignard@linaro.org; bo.song@mediatek.com;
>> matthias.bgg@gmail.com; labbott@redhat.com;
>> mingyuan.ma@mediatek.com; linux-arm-kernel@lists.infradead.org; linux-
>> media@vger.kernel.org
>> Subject: RE: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
>>
>>
>>> -----Original Message-----
>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>> guangming.cao@mediatek.com
>>> Sent: Thursday, January 13, 2022 7:34 AM
>>> To: sumit.semwal@linaro.org
>>> Cc: linux-arm-kernel@lists.infradead.org; mingyuan.ma@mediatek.com;
>>> Guangming <Guangming.Cao@mediatek.com>;
>>> wsd_upstream@mediatek.com; linux-kernel@vger.kernel.org; dri-
>>> devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org;
>>> yf.wang@mediatek.com; libo.kang@mediatek.com;
>>> benjamin.gaignard@linaro.org; bo.song@mediatek.com;
>>> matthias.bgg@gmail.com; linux-mediatek@lists.infradead.org;
>>> lmark@codeaurora.org; labbott@redhat.com; christian.koenig@amd.com;
>>> jianjiao.zeng@mediatek.com; linux-media@vger.kernel.org
>>> Subject: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
>>>
>>> From: Guangming <Guangming.Cao@mediatek.com>
>>>
>>> Add a size check for allocation since the allocation size is
>>> always less than the total DRAM size.
>>>
>>> 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.
>>>
>>> To make OOM issue easier, maybe need dma-buf framework to dump the
>>> buffer size
>>> under allocating in "dma_buf_debug_show".
>>>
>>> Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
>>> Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
>>> ---
>>> v3: 1. update patch, use right shift to replace division.
>>>     2. update patch, add reason in code and commit message.
>>> v2: 1. update size limitation as total_dram page size.
>>>     2. update commit message
>>> ---
>>> drivers/dma-buf/dma-heap.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>>> index 56bf5ad01ad5..1fd382712584 100644
>>> --- a/drivers/dma-buf/dma-heap.c
>>> +++ b/drivers/dma-buf/dma-heap.c
>>> @@ -55,6 +55,16 @@ static int dma_heap_buffer_alloc(struct dma_heap
>>> *heap, size_t len,
>>> 	struct dma_buf *dmabuf;
>>> 	int fd;
>>>
>>> +	/*
>>> +	 * 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.

In general I would rather let the system run into -ENOMEM or -EINVAL 
from the allocator instead.

Regards,
Christian.

>
> m
>> M
>>
>>> +		return -EINVAL;
>>> 	/*
>>> 	 * Allocations from all heaps have to begin
>>> 	 * and end on page boundaries.
>>> --
>>> 2.17.1

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

Posted by John Stultz 2 years, 8 months ago

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.

thanks
-john

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

Posted by Christian König 2 years, 8 months ago

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