[PATCH] spi: Fix memory leak on splited transfers

[PATCH] spi: Fix memory leak on splited transfers

[Gustav Wiklander]

From: Gustav Wiklander <gustavwi@axis.com>

In the prepare_message callback the bus driver has the
opportunity to split a transfer into smaller chunks.
spi_map_msg is done after prepare_message.

Function spi_res_release releases the splited transfers
in the message. Therefore spi_res_release should be called
after spi_map_msg.

The previous try at this was commit c9ba7a16d0f1
which released the splited transfers after
spi_finalize_current_message had been called.
This introduced a race since the message struct could be
out of scope because the spi_sync call got completed.

Fixes this leak on spi bus driver spi-bcm2835.c when transfer
size is greater than 65532:

[   76.611642][  T170] kmemleak: unreferenced object 0xfffffff06ef81480
(size 128):
[   76.618965][  T170] kmemleak:   comm "insmod", pid 493, jiffies
4294941102 (age 38.540s)
[   76.627031][  T170] kmemleak:   backtrace:
[   76.631206][  T170] kmemleak:     [<ffffffa542c5f8f8>]
create_object+0x100/0x288
[   76.638596][  T170] kmemleak:     [<ffffffa5432a9ee4>]
kmemleak_alloc+0x8c/0xe0
[   76.645723][  T170] kmemleak:     [<ffffffa542c4cbe8>]
__kmalloc+0x1c8/0x370
[   76.652754][  T170] kmemleak:     [<ffffffa542e14c94>]
sg_kmalloc+0x1c/0x68
[   76.659782][  T170] kmemleak:     [<ffffffa542e1543c>]
__sg_alloc_table+0xf4/0x128
[   76.667420][  T170] kmemleak:     [<ffffffa542e15820>]
sg_alloc_table+0x28/0xc8
[   76.674636][  T170] kmemleak:     [<ffffffa542f938a4>]
spi_map_buf+0xa4/0x300
[   76.681838][  T170] kmemleak:     [<ffffffa542f94648>]
__spi_pump_messages+0x370/0x748
[   76.689573][  T170] kmemleak:     [<ffffffa542f94c24>]
__spi_sync+0x1d4/0x270
[   76.696863][  T170] kmemleak:     [<ffffffa542f94cf4>]
spi_sync+0x34/0x58
[   76.703562][  T170] kmemleak:     [<ffffffa4cd94a638>]
spi_test_execute_msg+0x60/0x340 [spi_loopback_test]
[   76.713193][  T170] kmemleak:     [<ffffffa4cd94ae60>]
spi_test_run_iter+0x548/0x578 [spi_loopback_test]
[   76.722740][  T170] kmemleak:     [<ffffffa4cd94af24>]
spi_test_run_test+0x94/0x140 [spi_loopback_test]
[   76.732037][  T170] kmemleak:     [<ffffffa4cd94b120>]
spi_test_run_tests+0x150/0x180 [spi_loopback_test]
[   76.741498][  T170] kmemleak:     [<ffffffa4cd94b1a0>]
spi_loopback_test_probe+0x50/0xd0 [spi_loopback_test]
[   76.751392][  T170] kmemleak:     [<ffffffa542f911f4>]
spi_drv_probe+0x84/0xe0
---
 drivers/spi/spi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index dc12af018350..0cab239d8e7f 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1327,8 +1327,6 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 	if (msg->status && ctlr->handle_err)
 		ctlr->handle_err(ctlr, msg);
 
-	spi_res_release(ctlr, msg);
-
 	spi_finalize_current_message(ctlr);
 
 	return ret;
@@ -1725,6 +1723,13 @@ void spi_finalize_current_message(struct spi_controller *ctlr)
 
 	spi_unmap_msg(ctlr, mesg);
 
+	/* In the prepare_messages callback the spi bus has the opportunity to
+	 * split a transfer to smaller chunks.
+	 * Release splited transfers here since spi_map_msg is done on the
+	 * splited transfers.
+	 */
+	spi_res_release(ctlr, mesg);
+
 	if (ctlr->cur_msg_prepared && ctlr->unprepare_message) {
 		ret = ctlr->unprepare_message(ctlr, mesg);
 		if (ret) {
-- 
2.11.0

Re: [PATCH] spi: Fix memory leak on splited transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Sat, Sep 05, 2020 at 10:14:21PM +0200, Gustav Wiklander wrote:
> From: Gustav Wiklander <gustavwi@axis.com>
> 
> In the prepare_message callback the bus driver has the
> opportunity to split a transfer into smaller chunks.
> spi_map_msg is done after prepare_message.

You've not included a signed-off-by so I can't do anything with this,
see submitting-patches.rst for details on what this means and why it's
important.

> [   76.611642][  T170] kmemleak: unreferenced object 0xfffffff06ef81480
> (size 128):
> [   76.618965][  T170] kmemleak:   comm "insmod", pid 493, jiffies
> 4294941102 (age 38.540s)
> [   76.627031][  T170] kmemleak:   backtrace:
> [   76.631206][  T170] kmemleak:     [<ffffffa542c5f8f8>]

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]