| Message ID | 20211227095102.6054-1-guangming.cao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] dma-buf: dma-heap: Add a size check for allocation | expand |
On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote: > > From: Guangming <Guangming.Cao@mediatek.com> > Thanks for submitting this! > Add a size check for allcation since the allocation size is nit: "allocation" above. > always less than the total DRAM size. In general, it might be good to add more context to the commit message to better answer *why* this change is needed rather than what the change is doing. ie: What negative thing happens without this change? And so how does this change avoid or improve things? > Signed-off-by: Guangming <Guangming.Cao@mediatek.com> > Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com> > --- > v2: 1. update size limitation as total_dram page size. > 2. update commit message > --- > drivers/dma-buf/dma-heap.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 56bf5ad01ad5..e39d2be98d69 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, > struct dma_buf *dmabuf; > int fd; > > + if (len / PAGE_SIZE > totalram_pages()) > + return -EINVAL; This seems sane. I know ION used to have some 1/2 of memory cap to avoid unnecessary memory pressure on crazy allocations. Could you send again with an improved commit message? thanks -john
Am 03.01.22 um 19:57 schrieb John Stultz: > On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote: >> From: Guangming <Guangming.Cao@mediatek.com> >> > Thanks for submitting this! > >> Add a size check for allcation since the allocation size is > nit: "allocation" above. > >> always less than the total DRAM size. > In general, it might be good to add more context to the commit message > to better answer *why* this change is needed rather than what the > change is doing. ie: What negative thing happens without this change? > And so how does this change avoid or improve things? Completely agree, just one little addition: Could you also add this why as comment to the code? When we stumble over this five years from now it is absolutely not obvious why we do this. Thanks, Christian. > > >> Signed-off-by: Guangming <Guangming.Cao@mediatek.com> >> Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com> >> --- >> v2: 1. update size limitation as total_dram page size. >> 2. update commit message >> --- >> drivers/dma-buf/dma-heap.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c >> index 56bf5ad01ad5..e39d2be98d69 100644 >> --- a/drivers/dma-buf/dma-heap.c >> +++ b/drivers/dma-buf/dma-heap.c >> @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, >> struct dma_buf *dmabuf; >> int fd; >> >> + if (len / PAGE_SIZE > totalram_pages()) >> + return -EINVAL; > This seems sane. I know ION used to have some 1/2 of memory cap to > avoid unnecessary memory pressure on crazy allocations. > > Could you send again with an improved commit message? > > thanks > -john
On Tue, 2022-01-04 at 08:47 +0100, Christian König wrote: > Am 03.01.22 um 19:57 schrieb John Stultz: > > On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote: > > > From: Guangming <Guangming.Cao@mediatek.com> > > > > > > > Thanks for submitting this! > > > > > Add a size check for allcation since the allocation size is > > > > nit: "allocation" above. > > > > > always less than the total DRAM size. > > > > In general, it might be good to add more context to the commit > > message > > to better answer *why* this change is needed rather than what the > > change is doing. ie: What negative thing happens without this > > change? > > And so how does this change avoid or improve things? > > Completely agree, just one little addition: Could you also add this > why > as comment to the code? > > When we stumble over this five years from now it is absolutely not > obvious why we do this. Thanks for your reply! I will update the related reason in the patch later. The reason for adding this check is that we met a case that the user sent an invalid size(It seems it's a negative value, MSB is 0xff, it's larger than DRAM size after convert it to size_t) to dma-heap to alloc memory, and this allocation was running on a process(such as "gralloc" on Android device) can't be killed by OOM flow, and we also couldn't find the related dmabuf in "dma_buf_debug_show" because the related dmabuf was not exported yet since the allocation is still on going. Since this invalid argument case can be prevented at dma-heap side, so, I added this size check, and moreover, to let debug it easily, I also added logs when size is bigger than a threshold we set in mtk system heap. If you think that print logs in dma-heap framework is better, I will update it in next version. If you have better solution(such as dump the size under allocating in "dma_buf_debug_show", which maybe need add global variable to record it), please kindly let me know, thanks :) > > Thanks, > Christian. > > > > > > > > Signed-off-by: Guangming <Guangming.Cao@mediatek.com> > > > Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com> > > > --- > > > v2: 1. update size limitation as total_dram page size. > > > 2. update commit message > > > --- > > > drivers/dma-buf/dma-heap.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma- > > > heap.c > > > index 56bf5ad01ad5..e39d2be98d69 100644 > > > --- a/drivers/dma-buf/dma-heap.c > > > +++ b/drivers/dma-buf/dma-heap.c > > > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct > > > dma_heap *heap, size_t len, > > > struct dma_buf *dmabuf; > > > int fd; > > > > > > + if (len / PAGE_SIZE > totalram_pages()) > > > + return -EINVAL; > > > > This seems sane. I know ION used to have some 1/2 of memory cap to > > avoid unnecessary memory pressure on crazy allocations. > > > > Could you send again with an improved commit message? > > > > thanks > > -john > >
From: Guangming.Cao <guangming.cao@mediatek.com> On Tue, 2022-01-04 at 08:47 +0100, Christian K鰊ig wrote: > Am 03.01.22 um 19:57 schrieb John Stultz: > > On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote: > > > From: Guangming <Guangming.Cao@mediatek.com> > > > > > > > Thanks for submitting this! > > > > > Add a size check for allcation since the allocation size is > > > > nit: "allocation" above. > > > > > always less than the total DRAM size. > > > > In general, it might be good to add more context to the commit > > message > > to better answer *why* this change is needed rather than what the > > change is doing. ie: What negative thing happens without this > > change? > > And so how does this change avoid or improve things? > > Completely agree, just one little addition: Could you also add this > why > as comment to the code? > > When we stumble over this five years from now it is absolutely not > obvious why we do this. > > Thanks, > Christian. > Thanks for your reply! I will update the related reason in the patch later. The reason for adding this check is that we met a case that the user sent an invalid size(It seems it's a negative value, MSB is 0xff, it's larger than DRAM size after convert it to size_t) to dma-heap to alloc memory, and this allocation was running on a process(such as "gralloc" on Android device) can't be killed by OOM flow, and we also couldn't find the related dmabuf in "dma_buf_debug_show" because the related dmabuf was not exported yet since the allocation is still on going. Since this invalid argument case can be prevented at dma-heap side, so, I added this size check, and moreover, to let debug it easily, I also added logs when size is bigger than a threshold we set in mtk system heap. If you think that print logs in dma-heap framework is better, I will update it in next version. If you have better solution(such as dump the size under allocating in "dma_buf_debug_show", which maybe need add global variable to record it), please kindly let me know. Thanks :) Guangming > > > > > > > Signed-off-by: Guangming <Guangming.Cao@mediatek.com> > > > Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com> > > > --- > > > v2: 1. update size limitation as total_dram page size. > > > 2. update commit message > > > --- > > > drivers/dma-buf/dma-heap.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma- > > > heap.c > > > index 56bf5ad01ad5..e39d2be98d69 100644 > > > --- a/drivers/dma-buf/dma-heap.c > > > +++ b/drivers/dma-buf/dma-heap.c > > > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct > > > dma_heap *heap, size_t len, > > > struct dma_buf *dmabuf; > > > int fd; > > > > > > + if (len / PAGE_SIZE > totalram_pages()) > > > + return -EINVAL; > > > > This seems sane. I know ION used to have some 1/2 of memory cap to > > avoid unnecessary memory pressure on crazy allocations. > > > > Could you send again with an improved commit message? > > > > thanks > > -john > >
Hello Guangming, On Wed, 5 Jan 2022 at 12:05, <guangming.cao@mediatek.com> wrote: > > From: Guangming.Cao <guangming.cao@mediatek.com> > > On Tue, 2022-01-04 at 08:47 +0100, Christian K鰊ig wrote: > > Am 03.01.22 um 19:57 schrieb John Stultz: > > > On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote: > > > > From: Guangming <Guangming.Cao@mediatek.com> > > > > > > > > > > Thanks for submitting this! > > > > > > > Add a size check for allcation since the allocation size is > > > > > > nit: "allocation" above. > > > > > > > always less than the total DRAM size. > > > > > > In general, it might be good to add more context to the commit > > > message > > > to better answer *why* this change is needed rather than what the > > > change is doing. ie: What negative thing happens without this > > > change? > > > And so how does this change avoid or improve things? > > > > Completely agree, just one little addition: Could you also add this > > why > > as comment to the code? > > > > When we stumble over this five years from now it is absolutely not > > obvious why we do this. > > > > Thanks, > > Christian. > > > Thanks for your reply! > I will update the related reason in the patch later. > > The reason for adding this check is that we met a case that the user > sent an invalid size(It seems it's a negative value, MSB is 0xff, it's > larger than DRAM size after convert it to size_t) to dma-heap to alloc > memory, and this allocation was running on a process(such as "gralloc" > on Android device) can't be killed by OOM flow, and we also couldn't > find the related dmabuf in "dma_buf_debug_show" because the related > dmabuf was not exported yet since the allocation is still on going. > > Since this invalid argument case can be prevented at dma-heap side, so, > I added this size check, and moreover, to let debug it easily, I also > added logs when size is bigger than a threshold we set in mtk system > heap. > If you think that print logs in dma-heap framework is better, I will > update it in next version. > > If you have better solution(such as dump the size under allocating > in "dma_buf_debug_show", which maybe need add global variable to record > it), please kindly let me know. Thank you for the patch! I think just adding the reasoning above as the commit message and a comment in the code should be enough for now; the debug parts may be easy to add in case someone runs into issues. > Thanks :) > Guangming Best, Sumit. > > > > > > > > > > > Signed-off-by: Guangming <Guangming.Cao@mediatek.com> > > > > Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com> > > > > --- > > > > v2: 1. update size limitation as total_dram page size. > > > > 2. update commit message > > > > --- > > > > drivers/dma-buf/dma-heap.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma- > > > > heap.c > > > > index 56bf5ad01ad5..e39d2be98d69 100644 > > > > --- a/drivers/dma-buf/dma-heap.c > > > > +++ b/drivers/dma-buf/dma-heap.c > > > > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct > > > > dma_heap *heap, size_t len, > > > > struct dma_buf *dmabuf; > > > > int fd; > > > > > > > > + if (len / PAGE_SIZE > totalram_pages()) > > > > + return -EINVAL; > > > > > > This seems sane. I know ION used to have some 1/2 of memory cap to > > > avoid unnecessary memory pressure on crazy allocations. > > > > > > Could you send again with an improved commit message? > > > > > > thanks > > > -john > > > > -- Thanks and regards, Sumit Semwal (he / him) Tech Lead - LCG, Vertical Technologies Linaro.org │ Open source software for ARM SoCs
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 56bf5ad01ad5..e39d2be98d69 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, struct dma_buf *dmabuf; int fd; + if (len / PAGE_SIZE > totalram_pages()) + return -EINVAL; /* * Allocations from all heaps have to begin * and end on page boundaries.