Skip to contentSkip to conversation

rsi: Fix failure to load firmware after memory leak fix and fix the leak

PATCH Superseded Mike Looijmans 3 participants 6 messages Jul 27, 2015 – Jul 31, 2015

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rsi: Fix failure to load firmware after memory leak fix and fix the leak

Fixes commit eae79b4f3e82ca63a53478a161b190a0d38fe526 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Add a comment to the kmemdup() calls to explain why this is done.

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

---
 drivers/net/wireless/rsi/rsi_91x_sdio_ops.c | 6 +++++-
 drivers/net/wireless/rsi/rsi_91x_usb_ops.c  | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)
drivers/net/wireless/rsi/rsi_91x_sdio_ops.c +5 -1
index b6cc9ff..5c37a71 100644
@@ -172,6 +172,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		(struct rsi_91x_sdiodev *)adapter->rsi_dev;
 	u32 len;
 	u32 num_blocks;
+	const u8 *fw;
 	const struct firmware *fw_entry = NULL;
 	u32 block_size = dev->tx_blk_size;
 	int status = 0;
@@ -200,6 +201,8 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
+	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
 	len = fw_entry->size;
 
 	if (len % 4)
@@ -210,7 +213,8 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: Instruction size:%d\n", __func__, len);
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
-	status = rsi_copy_to_card(common, fw_entry->data, len, num_blocks);
+	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
drivers/net/wireless/rsi/rsi_91x_usb_ops.c +2 -0
index 1106ce7..088e28e 100644
@@ -146,6 +146,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
 	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
 	len = fw_entry->size;
 
@@ -158,6 +159,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
 	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
Signature · 6 lines
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reviewed-by on v1 1/1
#1 lore raw

suggestion. If we restore kmemdup() call, we have to handle ENOMEM situations:

	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
	if (!fw)
		return -ENOMEM;
Quoted 75 lines · Mike Looijmans

Fixes commit eae79b4f3e82ca63a53478a161b190a0d38fe526 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Add a comment to the kmemdup() calls to explain why this is done.

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

---
 drivers/net/wireless/rsi/rsi_91x_sdio_ops.c | 6 +++++-
 drivers/net/wireless/rsi/rsi_91x_usb_ops.c  | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)
drivers/net/wireless/rsi/rsi_91x_sdio_ops.c +5 -1
index b6cc9ff..5c37a71 100644
@@ -172,6 +172,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		(struct rsi_91x_sdiodev *)adapter->rsi_dev;
 	u32 len;
 	u32 num_blocks;
+	const u8 *fw;
 	const struct firmware *fw_entry = NULL;
 	u32 block_size = dev->tx_blk_size;
 	int status = 0;
@@ -200,6 +201,8 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
+	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
 	len = fw_entry->size;
 
 	if (len % 4)
@@ -210,7 +213,8 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: Instruction size:%d\n", __func__, len);
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
-	status = rsi_copy_to_card(common, fw_entry->data, len, num_blocks);
+	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
drivers/net/wireless/rsi/rsi_91x_usb_ops.c +2 -0
index 1106ce7..088e28e 100644
@@ -146,6 +146,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
 	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
 	len = fw_entry->size;
 
@@ -158,6 +159,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
 	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
#2 lore raw
In reply to Mike Looijmans
Quoted 2 lines · Mike Looijmans

Fixes commit eae79b4f3e82ca63a53478a161b190a0d38fe526 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

You can abbreviate the commit id:

Fixes commit eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Quoted 6 lines

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Quoted 8 lines

Add a comment to the kmemdup() calls to explain why this is done.

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

Add this before Signed-off-by line:

Fixes eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()")
Quoted 1 line

Also no need to send email to [email protected] list, this line is enough and the stable team will pick the commit automatically.

Signature · 1 line
Kalle Valo
#3 lore raw
Quoted 25 lines · Kalle Valo
Quoted 2 lines · Mike Looijmans

Fixes commit eae79b4f3e82ca63a53478a161b190a0d38fe526 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

You can abbreviate the commit id:

Fixes commit eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Quoted 6 lines

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Quoted 8 lines

Add a comment to the kmemdup() calls to explain why this is done.

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

Quoted 8 lines

Add this before Signed-off-by line:

Fixes eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()")
Quoted 1 line

Also no need to send email to [email protected] list, this line is enough and the stable team will pick the commit automatically.

I wondered why that happened, and just noticed that git send-email automatically added this to the recipients. So it happened for v2 as well, sorry for that.

Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: [email protected]
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

#4 lore raw

PATCH v2 1/1 rsi: Fix failure to load firmware after memory leak fix and fix the leak

In reply to Mike Looijmans

Fixes commit eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Add a comment to the kmemdup() calls to explain why this is done, and abort if memory allocation fails.

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

Fixes eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()")

---
v2: Add "Fixes:" header and abbreviate git hashes.
Return -ENOMEM if kmemdup() fails.

 drivers/net/wireless/rsi/rsi_91x_sdio_ops.c | 8 +++++++-
 drivers/net/wireless/rsi/rsi_91x_usb_ops.c  | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)
drivers/net/wireless/rsi/rsi_91x_sdio_ops.c +7 -1
index b6cc9ff..1c6788a 100644
@@ -172,6 +172,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		(struct rsi_91x_sdiodev *)adapter->rsi_dev;
 	u32 len;
 	u32 num_blocks;
+	const u8 *fw;
 	const struct firmware *fw_entry = NULL;
 	u32 block_size = dev->tx_blk_size;
 	int status = 0;
@@ -200,6 +201,10 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
+	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
 	len = fw_entry->size;
 
 	if (len % 4)
@@ -210,7 +215,8 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: Instruction size:%d\n", __func__, len);
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
-	status = rsi_copy_to_card(common, fw_entry->data, len, num_blocks);
+	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
drivers/net/wireless/rsi/rsi_91x_usb_ops.c +4 -0
index 1106ce7..30c2cf7 100644
@@ -146,7 +146,10 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 		return status;
 	}
 
+	/* Copy firmware into DMA-accessible memory */
 	fw = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
 	len = fw_entry->size;
 
 	if (len % 4)
@@ -158,6 +161,7 @@ static int rsi_load_ta_instructions(struct rsi_common *common)
 	rsi_dbg(INIT_ZONE, "%s: num blocks: %d\n", __func__, num_blocks);
 
 	status = rsi_copy_to_card(common, fw, len, num_blocks);
+	kfree(fw);
 	release_firmware(fw_entry);
 	return status;
 }
Signature · 1 line
1.9.1
Applied
#5 lore raw

Re: [v2] rsi: Fix failure to load firmware after memory leak fix and fixthe leak

Quoted 13 lines

Fixes commit eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()") which stopped the driver from functioning.

Firmware data has been allocated using vmalloc(), resulting in memory that cannot be used for DMA. Hence the firmware was first copied to a buffer allocated with kmalloc() in the original code. This patch reverts the commit and only calls "kfree()" to release the buffer after sending the data. This fixes the memory leak without breaking the driver.

Add a comment to the kmemdup() calls to explain why this is done, and abort if memory allocation fails.

Quoted 8 lines

Tested on a Topic Miami-Florida board which contains the rsi SDIO chip.

Also added the same kfree() call to the USB glue driver. This was not tested on actual hardware though, as I only have the SDIO version.

Fixes eae79b4f3e82 ("rsi: fix memory leak in rsi_load_ta_instructions()")

Thanks, applied to wireless-drivers.git.

Kalle Valo