From: Guangming Guangming.Cao@mediatek.com
Use (sg_table.orig_nents) rather than (sg_table.nents) to traverse sg_table to free sg_table. Use (sg_table.nents) maybe will casuse some pages can't be freed.
Signed-off-by: Guangming Guangming.Cao@mediatek.com --- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..ce10d4eb674c 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table; - for_each_sg(table->sgl, sg, table->nents, i) { + for_each_sg(table->sgl, sg, table->orig_nents, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
On 2021-11-25 12:46, guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
Use (sg_table.orig_nents) rather than (sg_table.nents) to traverse sg_table to free sg_table. Use (sg_table.nents) maybe will casuse some pages can't be freed.
...and this sort of bug is precisely why we have the for_each_sgtable_sg() helper ;)
Robin.
Signed-off-by: Guangming Guangming.Cao@mediatek.com
drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..ce10d4eb674c 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table;
- for_each_sg(table->sgl, sg, table->nents, i) {
for_each_sg(table->sgl, sg, table->orig_nents, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
From: Guangming Guangming.Cao@mediatek.com
Use (for_each_sgtable_sg) rather than (for_each_sg) to traverse sg_table to free sg_table. Use (for_each_sg) maybe will casuse some pages can't be freed when send wrong nents number.
Signed-off-by: Guangming Guangming.Cao@mediatek.com --- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table; - for_each_sg(table->sgl, sg, table->nents, i) { + for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
On 2021-11-25 13:49, guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
Use (for_each_sgtable_sg) rather than (for_each_sg) to traverse sg_table to free sg_table. Use (for_each_sg) maybe will casuse some pages can't be freed when send wrong nents number.
It's still worth spelling out that this is fixing a bug where the current code should have been using table->orig_nents - it's just that switching to the sgtable helper is the best way to make the fix, since it almost entirely removes the possibility of making that (previously rather common) mistake.
If it helps, for the change itself:
Reviewed-by: Robin Murphy robin.murphy@arm.com
Thanks, Robin.
Signed-off-by: Guangming Guangming.Cao@mediatek.com
drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table;
- for_each_sg(table->sgl, sg, table->nents, i) {
for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available")
Signed-off-by: Guangming Guangming.Cao@mediatek.com --- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table; - for_each_sg(table->sgl, sg, table->nents, i) { + for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
On Fri, Nov 26, 2021 at 11:16:05AM +0800, guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available")
Signed-off-by: Guangming Guangming.Cao@mediatek.com
drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table;
- for_each_sg(table->sgl, sg, table->nents, i) {
for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
-- 2.17.1
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") Signed-off-by: Guangming Guangming.Cao@mediatek.com Reviewed-by: Robin Murphy robin.murphy@arm.com Cc: stable@vger.kernel.org # 5.11.* --- v4: Correct commit message 1. Cc stable@vger.kernel.org in commit message and add required kernel version. 2. Add reviewed-by since patch V2 and V4 are same and V2 is reviewed by Robin. 3. There is no new code change in V4. V3: Cc stable@vger.kernel.org 1. This patch needs to be merged stable branch, add stable@vger.kernel.org in mail list. 2. Correct some spelling mistake. 3. There is No new code change in V3. V2: use 'for_each_sgtable_sg' to 'replece for_each_sg' as suggested by Robin.
--- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table; - for_each_sg(table->sgl, sg, table->nents, i) { + for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
Am 26.11.21 um 08:49 schrieb guangming.cao@mediatek.com:
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") Signed-off-by: Guangming Guangming.Cao@mediatek.com Reviewed-by: Robin Murphy robin.murphy@arm.com
Reviewed-by: Christian König christian.koenig@amd.com
Cc: stable@vger.kernel.org # 5.11.*
v4: Correct commit message 1. Cc stable@vger.kernel.org in commit message and add required kernel version. 2. Add reviewed-by since patch V2 and V4 are same and V2 is reviewed by Robin. 3. There is no new code change in V4. V3: Cc stable@vger.kernel.org 1. This patch needs to be merged stable branch, add stable@vger.kernel.org in mail list. 2. Correct some spelling mistake. 3. There is No new code change in V3. V2: use 'for_each_sgtable_sg' to 'replece for_each_sg' as suggested by Robin.
drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table;
- for_each_sg(table->sgl, sg, table->nents, i) {
for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
On Thu, Nov 25, 2021 at 11:48 PM guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") Signed-off-by: Guangming Guangming.Cao@mediatek.com Reviewed-by: Robin Murphy robin.murphy@arm.com Cc: stable@vger.kernel.org # 5.11.*
Thanks so much for catching this and sending in all the revisions!
Reviewed-by: John Stultz john.stultz@linaro.org
Hello Guangming,
On Mon, 29 Nov 2021 at 23:35, John Stultz john.stultz@linaro.org wrote:
On Thu, Nov 25, 2021 at 11:48 PM guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
For previous version, it uses 'sg_table.nent's to traverse sg_table in pages free flow. However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of created entries in the DMA adderess space. So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
Here we should use sg_table.orig_nents to free pages memory, but use the sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common helper 'for_each_sg' which maybe cause memory leak) is much better.
Thanks for catching this and the patch; applied to drm-misc-fixes.
Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") Signed-off-by: Guangming Guangming.Cao@mediatek.com Reviewed-by: Robin Murphy robin.murphy@arm.com Cc: stable@vger.kernel.org # 5.11.*
Thanks so much for catching this and sending in all the revisions!
Reviewed-by: John Stultz john.stultz@linaro.org
Best, Sumit.
On Thu, 2021-11-25 at 21:49 +0800, guangming.cao@mediatek.com wrote:
From: Guangming Guangming.Cao@mediatek.com
Use (for_each_sgtable_sg) rather than (for_each_sg) to traverse sg_table to free sg_table. Use (for_each_sg) maybe will casuse some pages can't be freed when send wrong nents number.
s/casuse/cause
Signed-off-by: Guangming Guangming.Cao@mediatek.com
drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma- buf/heaps/system_heap.c index 23a7e74ef966..8660508f3684 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) int i;
table = &buffer->sg_table;
- for_each_sg(table->sgl, sg, table->nents, i) {
for_each_sgtable_sg(table, sg, i) { struct page *page = sg_page(sg);
__free_pages(page, compound_order(page));
If this is a fix, I think we should add Fixes: xxxx ("...") in the commit message and Cc stable.
Thanks, Kuan-Ying Lee
dri-devel@lists.freedesktop.org