[v4,2/2] x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails

Commit Message

Ammar Faizi March 1, 2022, 9:46 a.m. UTC

From: Ammar Faizi <[e-mail]>

@bp is a local variable, calling mce_threshold_remove_device() when
threshold_create_bank() fails will not free the @bp. Note that
mce_threshold_remove_device() frees the @bp only if it's already
stored in the @threshold_banks per-CPU variable.

At that point, the @threshold_banks per-CPU variable is still NULL,
so the mce_threshold_remove_device() will just be a no-op and the
@bp is leaked.

Fix this by storing @bp to @threshold_banks before the loop, so in
case we fail, mce_threshold_remove_device() will free the @bp.

This bug is introduced by commit 6458de97fc15530b544 ("x86/mce/amd:
Straighten CPU hotplug path") [1].

Link: https://lore.kernel.org/all/[e-mail] [1]

v4:
  - Add the link to the commit reference again.

v3:
  - Fold in changes from Alviro, the previous version is still
    leaking @bank[n].

v2:
  - No changes.

Cc: Borislav Petkov <[e-mail]>
Cc: Dave Hansen <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: "H. Peter Anvin" <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Tony Luck <[e-mail]>
Cc: [e-mail] # v5.8+
Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
Co-authored-by: Alviro Iskandar Setiawan <[e-mail]>
Signed-off-by: Alviro Iskandar Setiawan <[e-mail]>
Signed-off-by: Ammar Faizi <[e-mail]>
---
 arch/x86/kernel/cpu/mce/amd.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Yazen Ghannam March 2, 2022, 5:26 p.m. UTC | #1

On Tue, Mar 01, 2022 at 04:46:08PM +0700, Ammar Faizi wrote:
> From: Ammar Faizi <[e-mail]>
> 

Hi Ammar,

...

> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 9f4b508886dd..a5ef161facd9 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -1346,19 +1346,23 @@ int mce_threshold_create_device(unsigned int cpu)
>  	if (!bp)
>  		return -ENOMEM;
>  
> +	/*
> +	 * If we fail, mce_threshold_remove_device() will free the @bp
> +	 * via @threshold_banks.
> +	 */
> +	this_cpu_write(threshold_banks, bp);
> +
>  	for (bank = 0; bank < numbanks; ++bank) {
>  		if (!(this_cpu_read(bank_map) & (1 << bank)))
>  			continue;
>  		err = threshold_create_bank(bp, cpu, bank);
> -		if (err)
> -			goto out_err;
> +		if (err) {
> +			mce_threshold_remove_device(cpu);
> +			return err;
> +		}
>  	}
> -	this_cpu_write(threshold_banks, bp);
>

The threshold interrupt handler uses this pointer. I think the goal here is to
set this pointer when the list is fully formed and clear this pointer before
making any changes to the list. Otherwise, the interrupt handler will operate
on incomplete data if an interrupt comes in the middle of these updates.

The changes below should deal with memory leak issue while avoiding a race
with the threshold interrupt. What do you think?

Thanks,
Yazen

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1940d305db1c..8f3b7859331d 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1294,10 +1294,22 @@ static void threshold_remove_bank(struct threshold_bank *bank)
 	kfree(bank);
 }
 
+void _mce_threshold_remove_device(struct threshold_bank **bp)
+{
+	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
+
+	for (bank = 0; bank < numbanks; bank++) {
+		if (bp[bank]) {
+			threshold_remove_bank(bp[bank]);
+			bp[bank] = NULL;
+		}
+	}
+	kfree(bp);
+}
+
 int mce_threshold_remove_device(unsigned int cpu)
 {
 	struct threshold_bank **bp = this_cpu_read(threshold_banks);
-	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
 
 	if (!bp)
 		return 0;
@@ -1308,13 +1320,7 @@ int mce_threshold_remove_device(unsigned int cpu)
 	 */
 	this_cpu_write(threshold_banks, NULL);
 
-	for (bank = 0; bank < numbanks; bank++) {
-		if (bp[bank]) {
-			threshold_remove_bank(bp[bank]);
-			bp[bank] = NULL;
-		}
-	}
-	kfree(bp);
+	_mce_threshold_remove_device(bp);
 	return 0;
 }
 
@@ -1360,6 +1366,6 @@ int mce_threshold_create_device(unsigned int cpu)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return 0;
 out_err:
-	mce_threshold_remove_device(cpu);
+	_mce_threshold_remove_device(bp);
 	return err;
 }

Ammar Faizi March 2, 2022, 11:20 p.m. UTC | #2

On 3/3/22 12:26 AM, Yazen Ghannam wrote:
> Hi Ammar,

Hi Yazen,

> ...
> The threshold interrupt handler uses this pointer. I think the goal here is to
> set this pointer when the list is fully formed and clear this pointer before
> making any changes to the list. Otherwise, the interrupt handler will operate
> on incomplete data if an interrupt comes in the middle of these updates.
> 
> The changes below should deal with memory leak issue while avoiding a race
> with the threshold interrupt. What do you think?

Thanks for taking a look into this. I didn't notice that before. The
changes look good to me, extra improvements:

1) _mce_threshold_remove_device() should be static as we don't use it
    in another translation unit.
2) Minor cleanup, we don't need "goto out_err", just early return
    directly.

I will fold them in...

Ammar Faizi March 2, 2022, 11:27 p.m. UTC | #3

On 3/3/22 6:20 AM, Ammar Faizi wrote:
> On 3/3/22 12:26 AM, Yazen Ghannam wrote:
>> Hi Ammar,
> 
> Hi Yazen,
> 
>> ...
>> The threshold interrupt handler uses this pointer. I think the goal here is to
>> set this pointer when the list is fully formed and clear this pointer before
>> making any changes to the list. Otherwise, the interrupt handler will operate
>> on incomplete data if an interrupt comes in the middle of these updates.
>>
>> The changes below should deal with memory leak issue while avoiding a race
>> with the threshold interrupt. What do you think?
> 
> Thanks for taking a look into this. I didn't notice that before. The
> changes look good to me, extra improvements:
> 
> 1) _mce_threshold_remove_device() should be static as we don't use it
>     in another translation unit.
> 2) Minor cleanup, we don't need "goto out_err", just early return
>     directly.
> 
> I will fold them in...
> 

Please review the patch below, if you think it looks good, I will
send this for the v5 series. I added your sign-off.

 From cae3965734a67d11a5286c612dfddf52398defc8 Mon Sep 17 00:00:00 2001
From: Ammar Faizi <[e-mail]>
Date: Thu, 3 Mar 2022 05:07:38 +0700
Subject: [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails

In mce_threshold_create_device(), when threshold_create_bank() fails,
the @bp will be leaked, because mce_threshold_remove_device() will
not free the @bp. It only frees the @bp when we've already written
the @bp to the @threshold_banks per-CPU variable, but at the point,
we haven't.

Fix this by extracting the cleanup part into a new static function
_mce_threshold_remove_device(), then use it from create and remove
device function.

Also, eliminate the "goto out_err". Just early return inside the loop
when we fail.

Cc: Borislav Petkov <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: [e-mail] # v5.8+
Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
Co-authored-by: Alviro Iskandar Setiawan <[e-mail]>
Signed-off-by: Alviro Iskandar Setiawan <[e-mail]>
Co-authored-by: Yazen Ghannam <[e-mail]>
Signed-off-by: Yazen Ghannam <[e-mail]>
Signed-off-by: Ammar Faizi <[e-mail]>
---
  arch/x86/kernel/cpu/mce/amd.c | 31 ++++++++++++++++++-------------
  1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9f4b508886dd..ac7246a4de08 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1293,10 +1293,22 @@ static void threshold_remove_bank(struct threshold_bank *bank)
  	kfree(bank);
  }
  
+static void _mce_threshold_remove_device(struct threshold_bank **bp)
+{
+	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
+
+	for (bank = 0; bank < numbanks; bank++) {
+		if (bp[bank]) {
+			threshold_remove_bank(bp[bank]);
+			bp[bank] = NULL;
+		}
+	}
+	kfree(bp);
+}
+
  int mce_threshold_remove_device(unsigned int cpu)
  {
  	struct threshold_bank **bp = this_cpu_read(threshold_banks);
-	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
  
  	if (!bp)
  		return 0;
@@ -1307,13 +1319,7 @@ int mce_threshold_remove_device(unsigned int cpu)
  	 */
  	this_cpu_write(threshold_banks, NULL);
  
-	for (bank = 0; bank < numbanks; bank++) {
-		if (bp[bank]) {
-			threshold_remove_bank(bp[bank]);
-			bp[bank] = NULL;
-		}
-	}
-	kfree(bp);
+	_mce_threshold_remove_device(bp);
  	return 0;
  }
  
@@ -1350,15 +1356,14 @@ int mce_threshold_create_device(unsigned int cpu)
  		if (!(this_cpu_read(bank_map) & (1 << bank)))
  			continue;
  		err = threshold_create_bank(bp, cpu, bank);
-		if (err)
-			goto out_err;
+		if (err) {
+			_mce_threshold_remove_device(bp);
+			return err;
+		}
  	}
  	this_cpu_write(threshold_banks, bp);
  
  	if (thresholding_irq_en)
  		mce_threshold_vector = amd_threshold_interrupt;
  	return 0;
-out_err:
-	mce_threshold_remove_device(cpu);
-	return err;
  }

Alviro Iskandar Setiawan March 3, 2022, 1:58 a.m. UTC | #4

On Thu, 3 Mar 2022 06:27:33 +0700, Ammar Faizi wrote:
> +static void _mce_threshold_remove_device(struct threshold_bank **bp)
> +{
> +	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
> +
> +	for (bank = 0; bank < numbanks; bank++) {
> +		if (bp[bank]) {
> +			threshold_remove_bank(bp[bank]);
> +			bp[bank] = NULL;
> +		}
> +	}
> +	kfree(bp);
> +}

hi sir, i think this can be improved again, we can avoid calling
this_cpu_read(mce_num_banks) twice if we pass the numbanks as an
argument, plz review the changes below

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9f4b508886dd..e492efab68a2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1293,10 +1293,23 @@ static void threshold_remove_bank(struct threshold_bank *bank)
 	kfree(bank);
 }
 
+static void _mce_threshold_remove_device(struct threshold_bank **bp,
+					 unsigned int numbanks)
+{
+	unsigned int bank;
+
+	for (bank = 0; bank < numbanks; bank++) {
+		if (bp[bank]) {
+			threshold_remove_bank(bp[bank]);
+			bp[bank] = NULL;
+		}
+	}
+	kfree(bp);
+}
+
 int mce_threshold_remove_device(unsigned int cpu)
 {
 	struct threshold_bank **bp = this_cpu_read(threshold_banks);
-	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
 
 	if (!bp)
 		return 0;
@@ -1307,13 +1320,7 @@ int mce_threshold_remove_device(unsigned int cpu)
 	 */
 	this_cpu_write(threshold_banks, NULL);
 
-	for (bank = 0; bank < numbanks; bank++) {
-		if (bp[bank]) {
-			threshold_remove_bank(bp[bank]);
-			bp[bank] = NULL;
-		}
-	}
-	kfree(bp);
+	_mce_threshold_remove_device(bp, this_cpu_read(mce_num_banks));
 	return 0;
 }
 
@@ -1350,15 +1357,14 @@ int mce_threshold_create_device(unsigned int cpu)
 		if (!(this_cpu_read(bank_map) & (1 << bank)))
 			continue;
 		err = threshold_create_bank(bp, cpu, bank);
-		if (err)
-			goto out_err;
+		if (err) {
+			_mce_threshold_remove_device(bp, numbanks);
+			return err;
+		}
 	}
 	this_cpu_write(threshold_banks, bp);
 
 	if (thresholding_irq_en)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return 0;
-out_err:
-	mce_threshold_remove_device(cpu);
-	return err;
 }

-- Viro

Ammar Faizi March 3, 2022, 2:07 a.m. UTC | #5

On 3/3/22 8:58 AM, Alviro Iskandar Setiawan wrote:
> hi sir, i think this can be improved again, we can avoid calling
> this_cpu_read(mce_num_banks) twice if we pass the numbanks as an
> argument, plz review the changes below

OK, nice improvement. I will fold this in...

Ammar Faizi March 3, 2022, 2:32 a.m. UTC | #6

On 3/3/22 9:07 AM, Ammar Faizi wrote:
> On 3/3/22 8:58 AM, Alviro Iskandar Setiawan wrote:
>> hi sir, i think this can be improved again, we can avoid calling
>> this_cpu_read(mce_num_banks) twice if we pass the numbanks as an
>> argument, plz review the changes below
> 
> OK, nice improvement. I will fold this in...
> 

It looks like this now. Yazen, Alviro, please review the
following patch. If you think it looks good, I will submit
it for the v5.

 From 91a447f837d502b7a040cd68f333fb98f4b941d9 Mon Sep 17 00:00:00 2001
From: Ammar Faizi <[e-mail]>
Date: Thu, 3 Mar 2022 09:22:17 +0700
Subject: [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails

In mce_threshold_create_device(), if threshold_create_bank() fails, the
@bp will be leaked, because mce_threshold_remove_device() will not free
the @bp. It only frees the @bp when we've already written the @bp to
the @threshold_banks per-CPU variable, but at the point, we haven't.

Fix this by extracting the cleanup part into a new static function
_mce_threshold_remove_device(), then use it from create/remove device
function. Also, eliminate the "goto out_err". Just early return inside
the loop if we fail.

Cc: Borislav Petkov <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: [e-mail] # v5.8+
Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
Co-authored-by: Alviro Iskandar Setiawan <[e-mail]>
Signed-off-by: Alviro Iskandar Setiawan <[e-mail]>
Co-authored-by: Yazen Ghannam <[e-mail]>
Signed-off-by: Yazen Ghannam <[e-mail]>
Signed-off-by: Ammar Faizi <[e-mail]>
---
  arch/x86/kernel/cpu/mce/amd.c | 32 +++++++++++++++++++-------------
  1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9f4b508886dd..e492efab68a2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1293,10 +1293,23 @@ static void threshold_remove_bank(struct threshold_bank *bank)
  	kfree(bank);
  }
  
+static void _mce_threshold_remove_device(struct threshold_bank **bp,
+					 unsigned int numbanks)
+{
+	unsigned int bank;
+
+	for (bank = 0; bank < numbanks; bank++) {
+		if (bp[bank]) {
+			threshold_remove_bank(bp[bank]);
+			bp[bank] = NULL;
+		}
+	}
+	kfree(bp);
+}
+
  int mce_threshold_remove_device(unsigned int cpu)
  {
  	struct threshold_bank **bp = this_cpu_read(threshold_banks);
-	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
  
  	if (!bp)
  		return 0;
@@ -1307,13 +1320,7 @@ int mce_threshold_remove_device(unsigned int cpu)
  	 */
  	this_cpu_write(threshold_banks, NULL);
  
-	for (bank = 0; bank < numbanks; bank++) {
-		if (bp[bank]) {
-			threshold_remove_bank(bp[bank]);
-			bp[bank] = NULL;
-		}
-	}
-	kfree(bp);
+	_mce_threshold_remove_device(bp, this_cpu_read(mce_num_banks));
  	return 0;
  }
  
@@ -1350,15 +1357,14 @@ int mce_threshold_create_device(unsigned int cpu)
  		if (!(this_cpu_read(bank_map) & (1 << bank)))
  			continue;
  		err = threshold_create_bank(bp, cpu, bank);
-		if (err)
-			goto out_err;
+		if (err) {
+			_mce_threshold_remove_device(bp, numbanks);
+			return err;
+		}
  	}
  	this_cpu_write(threshold_banks, bp);
  
  	if (thresholding_irq_en)
  		mce_threshold_vector = amd_threshold_interrupt;
  	return 0;
-out_err:
-	mce_threshold_remove_device(cpu);
-	return err;
  }

Alviro Iskandar Setiawan March 3, 2022, 2:51 a.m. UTC | #7

On Thu, Mar 3, 2022 at 9:32 AM Ammar Faizi wrote:
> On 3/3/22 9:07 AM, Ammar Faizi wrote:
> > On 3/3/22 8:58 AM, Alviro Iskandar Setiawan wrote:
> > > hi sir, i think this can be improved again, we can avoid calling
> > > this_cpu_read(mce_num_banks) twice if we pass the numbanks as an
> > > argument, plz review the changes below
> >
> > OK, nice improvement. I will fold this in...
> >
>
> It looks like this now. Yazen, Alviro, please review the
> following patch. If you think it looks good, I will submit
> it for the v5.
>

i think it looks good, thanks sir

-- Viro

Ammar Faizi March 7, 2022, 12:27 a.m. UTC | #8

On 3/3/22 9:32 AM, Ammar Faizi wrote:
> It looks like this now. Yazen, Alviro, please review the
> following patch. If you think it looks good, I will submit
> it for the v5.
> 
>  From 91a447f837d502b7a040cd68f333fb98f4b941d9 Mon Sep 17 00:00:00 2001
> From: Ammar Faizi <[e-mail]>
> Date: Thu, 3 Mar 2022 09:22:17 +0700
> Subject: [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails
> 
> In mce_threshold_create_device(), if threshold_create_bank() fails, the
> @bp will be leaked, because mce_threshold_remove_device() will not free
> the @bp. It only frees the @bp when we've already written the @bp to
> the @threshold_banks per-CPU variable, but at the point, we haven't.
> 
> Fix this by extracting the cleanup part into a new static function
> _mce_threshold_remove_device(), then use it from create/remove device
> function. Also, eliminate the "goto out_err". Just early return inside
> the loop if we fail.
> 
> Cc: Borislav Petkov <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: [e-mail] # v5.8+
> Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
> Co-authored-by: Alviro Iskandar Setiawan <[e-mail]>
> Signed-off-by: Alviro Iskandar Setiawan <[e-mail]>
> Co-authored-by: Yazen Ghannam <[e-mail]>
> Signed-off-by: Yazen Ghannam <[e-mail]>
> Signed-off-by: Ammar Faizi <[e-mail]>


Hi,

It's Monday morning...

Friendly ping for Yazen from AMD. What do you think of this patch? If
it looks good, I will submit it for the v5 revision. See the ref below
if you lost track of the full message.

Ref: https://lore.kernel.org/lkml/[e-mail]/

Thanks!

Yazen Ghannam March 9, 2022, 8:55 p.m. UTC | #9

On Mon, Mar 07, 2022 at 07:27:44AM +0700, Ammar Faizi wrote:
> On 3/3/22 9:32 AM, Ammar Faizi wrote:
> > It looks like this now. Yazen, Alviro, please review the
> > following patch. If you think it looks good, I will submit
> > it for the v5.
> > 
> >  From 91a447f837d502b7a040cd68f333fb98f4b941d9 Mon Sep 17 00:00:00 2001
> > From: Ammar Faizi <[e-mail]>
> > Date: Thu, 3 Mar 2022 09:22:17 +0700
> > Subject: [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails
> > 
> > In mce_threshold_create_device(), if threshold_create_bank() fails, the
> > @bp will be leaked, because mce_threshold_remove_device() will not free
> > the @bp. It only frees the @bp when we've already written the @bp to
> > the @threshold_banks per-CPU variable, but at the point, we haven't.
> > 
> > Fix this by extracting the cleanup part into a new static function
> > _mce_threshold_remove_device(), then use it from create/remove device
> > function. Also, eliminate the "goto out_err". Just early return inside
> > the loop if we fail.
> >

The commit message should use passive voice: no "I" or "we".

Otherwise, I think the patch looks good.

Thanks,
Yazen

Ammar Faizi March 10, 2022, 1:56 a.m. UTC | #10

On 3/10/22 3:55 AM, Yazen Ghannam wrote:
> The commit message should use passive voice: no "I" or "we".
> 
> Otherwise, I think the patch looks good.

Fixed in the v5 revision, thanks!

Link: https://lore.kernel.org/lkml/[e-mail]/

Patch

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9f4b508886dd..a5ef161facd9 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1346,19 +1346,23 @@  int mce_threshold_create_device(unsigned int cpu)
 	if (!bp)
 		return -ENOMEM;
 
+	/*
+	 * If we fail, mce_threshold_remove_device() will free the @bp
+	 * via @threshold_banks.
+	 */
+	this_cpu_write(threshold_banks, bp);
+
 	for (bank = 0; bank < numbanks; ++bank) {
 		if (!(this_cpu_read(bank_map) & (1 << bank)))
 			continue;
 		err = threshold_create_bank(bp, cpu, bank);
-		if (err)
-			goto out_err;
+		if (err) {
+			mce_threshold_remove_device(cpu);
+			return err;
+		}
 	}
-	this_cpu_write(threshold_banks, bp);
 
 	if (thresholding_irq_en)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return 0;
-out_err:
-	mce_threshold_remove_device(cpu);
-	return err;
 }