[PATCH 0/2] x86/xen: Fix memory leak issue

[PATCH 0/2] x86/xen: Fix memory leak issue

[Xiu Jianfeng]

The new string allocated by kasprintf() is leaked on error path

Xiu Jianfeng (2):
  x86/xen: Fix memory leak in xen_smp_intr_init{_pv}()
  x86/xen: Fix memory leak in xen_init_lock_cpu()

 arch/x86/xen/smp.c      | 16 ++++++++++++----
 arch/x86/xen/smp_pv.c   |  8 ++++++--
 arch/x86/xen/spinlock.c |  3 ++-
 3 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/2] x86/xen: Fix memory leak in xen_smp_intr_init{_pv}()

[Xiu Jianfeng]

These local variables @{resched|pmu|callfunc...}_name saves the new
string allocated by kasprintf(), and when bind_{v}ipi_to_irqhandler()
fails, it goes to the @fail tag, and calls xen_smp_intr_free{_pv}() to
free resource, however the new string is not saved, which cause a memory
leak issue. fix it.

Fixes: 9702785a747a ("i386: move xen")
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 arch/x86/xen/smp.c    | 16 ++++++++++++----
 arch/x86/xen/smp_pv.c |  8 ++++++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index c3e1f9a7d43a..6e9426b6b18a 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -71,8 +71,10 @@ int xen_smp_intr_init(unsigned int cpu)
 				    IRQF_PERCPU|IRQF_NOBALANCING,
 				    resched_name,
 				    NULL);
-	if (rc < 0)
+	if (rc < 0) {
+		kfree(resched_name);
 		goto fail;
+	}
 	per_cpu(xen_resched_irq, cpu).irq = rc;
 	per_cpu(xen_resched_irq, cpu).name = resched_name;
 
@@ -83,8 +85,10 @@ int xen_smp_intr_init(unsigned int cpu)
 				    IRQF_PERCPU|IRQF_NOBALANCING,
 				    callfunc_name,
 				    NULL);
-	if (rc < 0)
+	if (rc < 0) {
+		kfree(callfunc_name);
 		goto fail;
+	}
 	per_cpu(xen_callfunc_irq, cpu).irq = rc;
 	per_cpu(xen_callfunc_irq, cpu).name = callfunc_name;
 
@@ -94,8 +98,10 @@ int xen_smp_intr_init(unsigned int cpu)
 					     xen_debug_interrupt,
 					     IRQF_PERCPU | IRQF_NOBALANCING,
 					     debug_name, NULL);
-		if (rc < 0)
+		if (rc < 0) {
+			kfree(debug_name);
 			goto fail;
+		}
 		per_cpu(xen_debug_irq, cpu).irq = rc;
 		per_cpu(xen_debug_irq, cpu).name = debug_name;
 	}
@@ -107,8 +113,10 @@ int xen_smp_intr_init(unsigned int cpu)
 				    IRQF_PERCPU|IRQF_NOBALANCING,
 				    callfunc_name,
 				    NULL);
-	if (rc < 0)
+	if (rc < 0) {
+		kfree(callfunc_name);
 		goto fail;
+	}
 	per_cpu(xen_callfuncsingle_irq, cpu).irq = rc;
 	per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name;
 
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 480be82e9b7b..228d0207380c 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -124,8 +124,10 @@ int xen_smp_intr_init_pv(unsigned int cpu)
 				    IRQF_PERCPU|IRQF_NOBALANCING,
 				    callfunc_name,
 				    NULL);
-	if (rc < 0)
+	if (rc < 0) {
+		kfree(callfunc_name);
 		goto fail;
+	}
 	per_cpu(xen_irq_work, cpu).irq = rc;
 	per_cpu(xen_irq_work, cpu).name = callfunc_name;
 
@@ -135,8 +137,10 @@ int xen_smp_intr_init_pv(unsigned int cpu)
 					     xen_pmu_irq_handler,
 					     IRQF_PERCPU|IRQF_NOBALANCING,
 					     pmu_name, NULL);
-		if (rc < 0)
+		if (rc < 0) {
+			kfree(pmu_name);
 			goto fail;
+		}
 		per_cpu(xen_pmu_irq, cpu).irq = rc;
 		per_cpu(xen_pmu_irq, cpu).name = pmu_name;
 	}
-- 
2.17.1

[PATCH 2/2] x86/xen: Fix memory leak in xen_init_lock_cpu()

[Xiu Jianfeng]

In xen_init_lock_cpu(), the @name has allocated new string by kasprintf(),
if bind_ipi_to_irqhandler() fails, it should be freed, otherwise may lead
to a memory leak issue, fix it.

Fixes: 2d9e1e2f58b5 ("xen: implement Xen-specific spinlocks")
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 arch/x86/xen/spinlock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 043c73dfd2c9..156d3e04c9ef 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -86,7 +86,8 @@ void xen_init_lock_cpu(int cpu)
 		disable_irq(irq); /* make sure it's never delivered */
 		per_cpu(lock_kicker_irq, cpu) = irq;
 		per_cpu(irq_name, cpu) = name;
-	}
+	} else
+		kfree(name);
 
 	printk("cpu %d spinlock event irq %d\n", cpu, irq);
 }
-- 
2.17.1

Re: [PATCH 0/2] x86/xen: Fix memory leak issue

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 735 bytes --]

On 19.11.22 09:59, Xiu Jianfeng wrote:
> The new string allocated by kasprintf() is leaked on error path
> 
> Xiu Jianfeng (2):
>    x86/xen: Fix memory leak in xen_smp_intr_init{_pv}()
>    x86/xen: Fix memory leak in xen_init_lock_cpu()
> 
>   arch/x86/xen/smp.c      | 16 ++++++++++++----
>   arch/x86/xen/smp_pv.c   |  8 ++++++--
>   arch/x86/xen/spinlock.c |  3 ++-
>   3 files changed, 20 insertions(+), 7 deletions(-)
> 

Hmm, I think it would make more sense to always store the name generated
via kasprintf() in the percpu variable (independently from succeeding to
bind the irq), and in the related free function to always kfree() it and
set it to NULL again.

This would result in less code.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/2] x86/xen: Fix memory leak issue

[xiujianfeng]

Hi,

在 2022/11/23 23:23, Juergen Gross 写道:
> On 19.11.22 09:59, Xiu Jianfeng wrote:
>> The new string allocated by kasprintf() is leaked on error path
>>
>> Xiu Jianfeng (2):
>>    x86/xen: Fix memory leak in xen_smp_intr_init{_pv}()
>>    x86/xen: Fix memory leak in xen_init_lock_cpu()
>>
>>   arch/x86/xen/smp.c      | 16 ++++++++++++----
>>   arch/x86/xen/smp_pv.c   |  8 ++++++--
>>   arch/x86/xen/spinlock.c |  3 ++-
>>   3 files changed, 20 insertions(+), 7 deletions(-)
>>
> 
> Hmm, I think it would make more sense to always store the name generated
> via kasprintf() in the percpu variable (independently from succeeding to
> bind the irq), and in the related free function to always kfree() it and
> set it to NULL again.
> 
> This would result in less code.

Thanks, It's good to me, already sent v2.
> 
> 
> Juergen