Re: [PATCH 2/2] drivers: staging: wilc1000: Call kfree only for error cases

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2015년 10월 04일 19:28, Chandra Gorentla wrote:
On Sun, Oct 04, 2015 at 09:44:57AM +0100, Greg KH wrote:
On Sat, Oct 03, 2015 at 02:57:30PM +0530, Chandra S Gorentla wrote:
  - kfree is being called for the members of the queue without
    de-queuing them; they are just inserted within this function;
    they are supposed to be de-queued and freed in a function
    for receiving the queue items
  - goto statements are removed
  - After kfree correction, there is no need for target block
    of goto statement; hence it is removed

Signed-off-by: Chandra S Gorentla <csgorentla@xxxxxxxxx>
---
  drivers/staging/wilc1000/wilc_msgqueue.c | 22 ++++++----------------
  1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c
index 284a3f5..eae90be 100644
--- a/drivers/staging/wilc1000/wilc_msgqueue.c
+++ b/drivers/staging/wilc1000/wilc_msgqueue.c
@@ -56,32 +56,30 @@ int wilc_mq_destroy(WILC_MsgQueueHandle *pHandle)
  int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
  			     const void *pvSendBuffer, u32 u32SendBufferSize)
  {
-	int result = 0;
  	unsigned long flags;
  	Message *pstrMessage = NULL;
if ((!pHandle) || (u32SendBufferSize == 0) || (!pvSendBuffer)) {
  		PRINT_ER("pHandle or pvSendBuffer is null\n");
-		result = -EFAULT;
-		goto ERRORHANDLER;
+		return -EFAULT;
  	}
if (pHandle->bExiting) {
  		PRINT_ER("pHandle fail\n");
-		result = -EFAULT;
-		goto ERRORHANDLER;
+		return -EFAULT;
  	}
/* construct a new message */
  	pstrMessage = kmalloc(sizeof(Message), GFP_ATOMIC);
  	if (!pstrMessage)
  		return -ENOMEM;
+
  	pstrMessage->u32Length = u32SendBufferSize;
  	pstrMessage->pstrNext = NULL;
  	pstrMessage->pvBuffer = kmalloc(u32SendBufferSize, GFP_ATOMIC);
  	if (!pstrMessage->pvBuffer) {
-		result = -ENOMEM;
-		goto ERRORHANDLER;
+		kfree(pstrMessage);
+		return -ENOMEM;
  	}
  	memcpy(pstrMessage->pvBuffer, pvSendBuffer, u32SendBufferSize);
@@ -102,15 +100,7 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
  	spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
up(&pHandle->hSem);
-
-ERRORHANDLER:
-	/* error occured, free any allocations */
-	if (pstrMessage) {
-		kfree(pstrMessage->pvBuffer);
-		kfree(pstrMessage);
-	}
-
-	return result;
+	return 0;
Aren't you now leaking memory as you aren't freeing pstrMessage and the
buffer on the "normal" return path?
In the normal path kfree is called in a separate (wilc_mq_recv) function.
The purpose of the currently modified function (wilc_mq_send) is to post
a message to a queue by allocating memory for the message.  The receiver
function is supposed to remove the message from the queue and free the
memory.

This patch is reasonable and normal free is done in recv function as Chandra said.

Thanks,

Tony.

thanks,

greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux