Skip to contentSkip to conversation

Drivers: hv: vmbus: Call hv_synic_free() if hv_synic_alloc() fails

PATCH Applied Dexuan Cui 3 participants 4 messages May 4, 2023 – May 8, 2023

Subsystems (2)
Hyper-V/Azure core and drivers Microsoft MANA RDMA driver
lore
Share & tools
lore

Dexuan Cui

author maintainer
#0 lore raw

PATCH 1/1 Drivers: hv: vmbus: Call hv_synic_free() if hv_synic_alloc() fails

Commit 572086325ce9 ("Drivers: hv: vmbus: Cleanup synic memory free path") says "Any memory allocations that succeeded will be freed when the caller cleans up by calling hv_synic_free()", but if the get_zeroed_page() in hv_synic_alloc() fails, currently hv_synic_free() is not really called in vmbus_bus_init(), consequently there will be a memory lead, e.g. hv_context.hv_numa_map is not freed in the error path. Fix this by updating the goto lables.

Cc [email protected]
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/vmbus_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/hv/vmbus_drv.c +2 -3
index 59eb5fad12e7..c41e6ad0cf64 100644
@@ -1525,7 +1525,7 @@ static int vmbus_bus_init(void)
 	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
 				hv_synic_init, hv_synic_cleanup);
 	if (ret < 0)
-		goto err_cpuhp;
+		goto err_alloc;
 	hyperv_cpuhp_online = ret;
 
 	ret = vmbus_connect();

@@ -1577,9 +1577,8 @@ static int vmbus_bus_init(void)
 
 err_connect:
 	cpuhp_remove_state(hyperv_cpuhp_online);
-err_cpuhp:
-	hv_synic_free();
 err_alloc:
+	hv_synic_free();
 	if (vmbus_irq == -1) {
 		hv_remove_vmbus_handler();
 	} else {
Signature · 1 line
2.25.1
Reviewed-by on v1 1/1
#1 lore raw

From: Dexuan Cui <[email protected]> Sent: Thursday, May 4, 2023 3:42 PM

Quoted 6 lines

Commit 572086325ce9 ("Drivers: hv: vmbus: Cleanup synic memory free path") says "Any memory allocations that succeeded will be freed when the caller cleans up by calling hv_synic_free()", but if the get_zeroed_page() in hv_synic_alloc() fails, currently hv_synic_free() is not really called in vmbus_bus_init(), consequently there will be a memory lead, e.g.

s/lead/leak/

Quoted 2 lines

hv_context.hv_numa_map is not freed in the error path. Fix this by updating the goto lables.

s/lables/labels/

The statement in commit 572086325ce9 was true at the time of that commit. But I broke things in commit 4df4cb9e99f8. Should add a "Fixes:" tag for 4df4cb9e99f8.

Quoted 32 lines
Cc [email protected]
Signed-off-by Dexuan Cui <[email protected]>
---
 drivers/hv/vmbus_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/hv/vmbus_drv.c +2 -4
index 59eb5fad12e7..c41e6ad0cf64 100644
@@ -1525,7 +1525,7 @@ static int vmbus_bus_init(void)
 	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
 				hv_synic_init, hv_synic_cleanup);
 	if (ret < 0)
-		goto err_cpuhp;
+		goto err_alloc;
 	hyperv_cpuhp_online = ret;
 
 	ret = vmbus_connect();

@@ -1577,9 +1577,8 @@ static int vmbus_bus_init(void)
 
 err_connect:
 	cpuhp_remove_state(hyperv_cpuhp_online);
-err_cpuhp:
-	hv_synic_free();
 err_alloc:
+	hv_synic_free();
 	if (vmbus_irq == -1) {
 		hv_remove_vmbus_handler();
 	} else {
--
Quoted 1 line

2.25.1

Nits notwithstanding,

Reviewed-by Michael Kelley <[email protected]>
author maintainer
#2 lore raw
Quoted 6 lines

From: Michael Kelley (LINUX) <[email protected]>
Sent: Friday, May 5, 2023 5:30 PM
...
From: Dexuan Cui <[email protected]> Sent: Thursday, May 4, 2023 3:42 PM

Quoted 1 line
Quoted 8 lines
Quoted 6 lines

Commit 572086325ce9 ("Drivers: hv: vmbus: Cleanup synic memory free path")
says "Any memory allocations that succeeded will be freed when the caller cleans up by calling hv_synic_free()", but if the get_zeroed_page() in hv_synic_alloc() fails, currently hv_synic_free() is not really called in vmbus_bus_init(), consequently there will be a memory lead, e.g.

s/lead/leak/

Sorry for the typo. Wei, can you please help fix this?

Quoted 4 lines
Quoted 2 lines

hv_context.hv_numa_map is not freed in the error path. Fix this by updating the goto lables.

s/lables/labels/

Ditto.

Quoted 3 lines

The statement in commit 572086325ce9 was true at the time of that commit. But I broke things in commit 4df4cb9e99f8. Should add a "Fixes:" tag for 4df4cb9e99f8.

I suppose Wei can help add the line:

Fixes 4df4cb9e99f8 ("x86/hyperv: Initialize clockevents earlier in CPU onlining")
Quoted 3 lines

Nits notwithstanding,

Reviewed-by Michael Kelley <[email protected]>

Thanks!

maintainer Applied
#3 lore raw
Quoted 21 lines · Dexuan Cui
Quoted 6 lines

From: Michael Kelley (LINUX) <[email protected]>
Sent: Friday, May 5, 2023 5:30 PM
...
From: Dexuan Cui <[email protected]> Sent: Thursday, May 4, 2023 3:42 PM

Quoted 1 line
Quoted 8 lines
Quoted 6 lines

Commit 572086325ce9 ("Drivers: hv: vmbus: Cleanup synic memory free path")
says "Any memory allocations that succeeded will be freed when the caller cleans up by calling hv_synic_free()", but if the get_zeroed_page() in hv_synic_alloc() fails, currently hv_synic_free() is not really called in vmbus_bus_init(), consequently there will be a memory lead, e.g.

s/lead/leak/

Sorry for the typo. Wei, can you please help fix this?

Quoted 4 lines
Quoted 2 lines

hv_context.hv_numa_map is not freed in the error path. Fix this by updating the goto lables.

s/lables/labels/

Quoted 12 lines

Ditto.

Quoted 3 lines

The statement in commit 572086325ce9 was true at the time of that commit. But I broke things in commit 4df4cb9e99f8. Should add a "Fixes:" tag for 4df4cb9e99f8.

I suppose Wei can help add the line:

Fixes 4df4cb9e99f8 ("x86/hyperv: Initialize clockevents earlier in CPU onlining")
Quoted 3 lines

Nits notwithstanding,

Reviewed-by Michael Kelley <[email protected]>

Applied to hyperv-fixes. Thanks.