Skip to contentSkip to conversation

Drivers: hv: balloon: Fix a memory leak

PATCH In review K. Y. Srinivasan 3 participants 4 messages Dec 11, 2012 – Dec 13, 2012 Hyper-V/Azure core and drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Drivers: hv: balloon: Fix a memory leak

The send buffer was being leaked; fix it.

---
 drivers/hv/hv_balloon.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
drivers/hv/hv_balloon.c +13 -10
index 00c8447..c02a33e 100644
@@ -882,7 +882,7 @@ static int balloon_probe(struct hv_device *dev,
 			balloon_onchannelcallback, dev);
 
 	if (ret)
-		return ret;
+		goto probe_error0;
 
 	dm_device.dev = dev;
 	dm_device.state = DM_INITIALIZING;
@@ -894,7 +894,7 @@ static int balloon_probe(struct hv_device *dev,
 		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
 	if (IS_ERR(dm_device.thread)) {
 		ret = PTR_ERR(dm_device.thread);
-		goto probe_error0;
+		goto probe_error1;
 	}
 
 	hv_set_drvdata(dev, &dm_device);
@@ -917,12 +917,12 @@ static int balloon_probe(struct hv_device *dev,
 				VM_PKT_DATA_INBAND,
 				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret)
-		goto probe_error1;
+		goto probe_error2;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	/*
@@ -931,7 +931,7 @@ static int balloon_probe(struct hv_device *dev,
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 	/*
 	 * Now submit our capabilities to the host.
@@ -964,12 +964,12 @@ static int balloon_probe(struct hv_device *dev,
 				VM_PKT_DATA_INBAND,
 				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret)
-		goto probe_error1;
+		goto probe_error2;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	/*
@@ -978,18 +978,20 @@ static int balloon_probe(struct hv_device *dev,
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	dm_device.state = DM_INITIALIZED;
 
 	return 0;
 
-probe_error1:
+probe_error2:
 	kthread_stop(dm_device.thread);
 
-probe_error0:
+probe_error1:
 	vmbus_close(dev->channel);
+probe_error0:
+	kfree(send_buffer);
 	return ret;
 }
 
@@ -1002,6 +1004,7 @@ static int balloon_remove(struct hv_device *dev)
 
 	vmbus_close(dev->channel);
 	kthread_stop(dm->thread);
+	kfree(send_buffer);
 
 	return 0;
 }
Signature · 1 line
1.7.4.1
Acked-by on v1 1/1
#1 lore raw
Quoted 8 lines · K. Y. Srinivasan

The send buffer was being leaked; fix it.

---
 drivers/hv/hv_balloon.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

Thanks.

Quoted 93 lines
drivers/hv/hv_balloon.c +13 -10
index 00c8447..c02a33e 100644
@@ -882,7 +882,7 @@ static int balloon_probe(struct hv_device *dev,
 			balloon_onchannelcallback, dev);
 
 	if (ret)
-		return ret;
+		goto probe_error0;
 
 	dm_device.dev = dev;
 	dm_device.state = DM_INITIALIZING;
@@ -894,7 +894,7 @@ static int balloon_probe(struct hv_device *dev,
 		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
 	if (IS_ERR(dm_device.thread)) {
 		ret = PTR_ERR(dm_device.thread);
-		goto probe_error0;
+		goto probe_error1;
 	}
 
 	hv_set_drvdata(dev, &dm_device);
@@ -917,12 +917,12 @@ static int balloon_probe(struct hv_device *dev,
 				VM_PKT_DATA_INBAND,
 				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret)
-		goto probe_error1;
+		goto probe_error2;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	/*
@@ -931,7 +931,7 @@ static int balloon_probe(struct hv_device *dev,
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 	/*
 	 * Now submit our capabilities to the host.
@@ -964,12 +964,12 @@ static int balloon_probe(struct hv_device *dev,
 				VM_PKT_DATA_INBAND,
 				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret)
-		goto probe_error1;
+		goto probe_error2;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	/*
@@ -978,18 +978,20 @@ static int balloon_probe(struct hv_device *dev,
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
 		ret = -ETIMEDOUT;
-		goto probe_error1;
+		goto probe_error2;
 	}
 
 	dm_device.state = DM_INITIALIZED;
 
 	return 0;
 
-probe_error1:
+probe_error2:
 	kthread_stop(dm_device.thread);
 
-probe_error0:
+probe_error1:
 	vmbus_close(dev->channel);
+probe_error0:
+	kfree(send_buffer);
 	return ret;
 }
 
@@ -1002,6 +1004,7 @@ static int balloon_remove(struct hv_device *dev)
 
 	vmbus_close(dev->channel);
 	kthread_stop(dm->thread);
+	kfree(send_buffer);
 
 	return 0;
 }
#2 lore raw
In reply to K. Y. Srinivasan

drivers/hv/hv_balloon.c static int balloon_probe(struct hv_device *dev, · line 994

Quoted 9 lines · K. Y. Srinivasan
drivers/hv/hv_balloon.c +4 -2
-probe_error1:
+probe_error2:
 	kthread_stop(dm_device.thread);
 
-probe_error0:
+probe_error1:
 	vmbus_close(dev->channel);
+probe_error0:
+	kfree(send_buffer);

If you used labels that describe what the code does instead of GW-BASIC labels then you wouldn't need to rename them.

err_stop_thread:

	kthread_stop(dm_device.thread);
err_close_vmbus:
	vmbus_close(dev->channel);
err_free_send_buf:
	kfree(send_buffer);

regards,
dan carpenter

#3 lore raw
Quoted 25 lines

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, December 13, 2012 4:38 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 1/1] Drivers: hv: balloon: Fix a memory leak

drivers/hv/hv_balloon.c static int balloon_probe(struct hv_device *dev, · line 994

Quoted 9 lines · K. Y. Srinivasan
drivers/hv/hv_balloon.c +4 -2
-probe_error1:
+probe_error2:
 	kthread_stop(dm_device.thread);
 
-probe_error0:
+probe_error1:
 	vmbus_close(dev->channel);
+probe_error0:
+	kfree(send_buffer);

If you used labels that describe what the code does instead of GW-BASIC labels then you wouldn't need to rename them.

Quoted 6 lines

err_stop_thread:

	kthread_stop(dm_device.thread);
err_close_vmbus:
	vmbus_close(dev->channel);
err_free_send_buf:
	kfree(send_buffer);

Thanks Dan. I am not sure how this labeling scheme would work in general where each error recovery block is a collection of rollback operation. I see your point though.

Regards,

K. Y