Hi,
Two fixes for x86 arch.
## Changelog
v4: - Address comment from Greg, sha1 commit Fixes only needs to be 12 chars. - Add the author of the fixed commit to the CC list.
v3: - Fold in changes from Alviro, the previous version is still leaking @bank[n].
v2: - Fix wrong copy/paste.
## Short Summary
Patch 1, fixes the wrong asm constraint in delay_loop function.
Fortunately, the constraint violation that's fixed by patch 1 doesn't yield any bug due to the nature of System V ABI. Should we backport this?
Patch 2, fixes memory leak in mce/amd code.
Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "H. Peter Anvin" hpa@zytor.com Cc: Ingo Molnar mingo@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Tony Luck tony.luck@intel.com Signed-off-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org --- Ammar Faizi (2): x86/delay: Fix the wrong asm constraint in `delay_loop()` x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails
arch/x86/kernel/cpu/mce/amd.c | 16 ++++++++++------ arch/x86/lib/delay.c | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-)
base-commit: 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
From: Ammar Faizi ammarfaizi2@gnuweeb.org
The asm constraint does not reflect that the asm statement can modify the value of @loops. But the asm statement in delay_loop() does change the @loops.
If by any chance the compiler inlines this function, it may clobber random stuff (e.g. local variable, important temporary value in reg, etc.).
Fortunately, delay_loop() is only called indirectly (so it can't inline), and then the register it clobbers is %rax (which is by the nature of the calling convention, it's a caller saved register), so it didn't yield any bug.
^ That shouldn't be an excuse for using the wrong constraint anyway.
This changes "a" (as an input) to "+a" (as an input and output).
Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: Jiri Hladky hladky.jiri@googlemail.com Fixes: e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function") Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org ---
Fortunately, the constraint violation that's fixed by patch 1 doesn't yield any bug due to the nature of System V ABI. Should we backport this?
arch/x86/lib/delay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c index 65d15df6212d..0e65d00e2339 100644 --- a/arch/x86/lib/delay.c +++ b/arch/x86/lib/delay.c @@ -54,8 +54,8 @@ static void delay_loop(u64 __loops) " jnz 2b \n" "3: dec %0 \n"
- : /* we don't need output */ - :"a" (loops) + : "+a" (loops) + : ); }
From: Ammar Faizi
Sent: 01 March 2022 09:46
The asm constraint does not reflect that the asm statement can modify the value of @loops. But the asm statement in delay_loop() does change the @loops.
If by any chance the compiler inlines this function, it may clobber random stuff (e.g. local variable, important temporary value in reg, etc.).
Fortunately, delay_loop() is only called indirectly (so it can't inline), and then the register it clobbers is %rax (which is by the nature of the calling convention, it's a caller saved register), so it didn't yield any bug.
Both the function pointers in that code need killing. They only have two options (each) so conditional branches will almost certainly always have been better.
I also wonder how well the comment The additional jump magic is needed to get the timing stable on all the CPU' we have to worry about. applies to any modern cpu! The code is unchanged since (at least) 2.6.27. (It might have been moved from another file.)
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 3/1/22 4:54 PM, David Laight wrote:
Both the function pointers in that code need killing. They only have two options (each) so conditional branches will almost certainly always have been better.
Yes, I agree with simply using conditional branches to handle this case. But to keep the changes minimal for the stable tree, let's fix the obvious real bug first. Someone can refactor it later, but I don't see that as an urgent thing to refactor.
I also wonder how well the comment The additional jump magic is needed to get the timing stable on all the CPU' we have to worry about. applies to any modern cpu! The code is unchanged since (at least) 2.6.27. (It might have been moved from another file.)
Not sure about that...
Thanks for the feedback.
On Tue, Mar 1, 2022 at 4:46 PM Ammar Faizi wrote:
Fortunately, the constraint violation that's fixed by patch 1 doesn't yield any bug due to the nature of System V ABI. Should we backport this?
hi sir, it might also be interesting to know that even if it never be inlined, it's still potential to break.
for example this code (https://godbolt.org/z/xWMTxhTET)
__attribute__((__noinline__)) static void x(int a) { asm("xorl\t%%r8d, %%r8d"::"a"(a)); }
extern int p(void);
int f(void) { int ret = p(); x(ret); return ret; }
translates to this asm
x: movl %edi, %eax xorl %r8d, %r8d ret f: subq $8, %rsp call p movl %eax, %r8d movl %eax, %edi call x movl %r8d, %eax addq $8, %rsp ret
See the %r8d? It should be clobbered by a function call too. But since no one tells the compiler that we clobber %r8d, it assumes %r8d never changes after that call. The compiler thinks x() is static and will not clobber %r8d, even the ABI says %r8d will be clobbered by a function call. So i think it should be backported to the stable kernel, it's still a fix
-- Viro
On 3/1/22 6:33 PM, Alviro Iskandar Setiawan wrote:
hi sir, it might also be interesting to know that even if it never be inlined, it's still potential to break.
for example this code (https://godbolt.org/z/xWMTxhTET)
__attribute__((__noinline__)) static void x(int a) { asm("xorl\t%%r8d, %%r8d"::"a"(a)); }
extern int p(void);
int f(void) { int ret = p(); x(ret); return ret; }
translates to this asm
x: movl %edi, %eax xorl %r8d, %r8d ret f: subq $8, %rsp call p movl %eax, %r8d movl %eax, %edi call x movl %r8d, %eax addq $8, %rsp ret
See the %r8d? It should be clobbered by a function call too. But since no one tells the compiler that we clobber %r8d, it assumes %r8d never changes after that call. The compiler thinks x() is static and will not clobber %r8d, even the ABI says %r8d will be clobbered by a function call. So i think it should be backported to the stable kernel, it's still a fix
Thanks. I will add CC stable in the v5.
From: Alviro Iskandar Setiawan
Sent: 01 March 2022 11:34
On Tue, Mar 1, 2022 at 4:46 PM Ammar Faizi wrote:
Fortunately, the constraint violation that's fixed by patch 1 doesn't yield any bug due to the nature of System V ABI. Should we backport this?
hi sir, it might also be interesting to know that even if it never be inlined, it's still potential to break.
for example this code (https://godbolt.org/z/xWMTxhTET)
__attribute__((__noinline__)) static void x(int a) { asm("xorl\t%%r8d, %%r8d"::"a"(a)); }
But this code isn't doing that. In your example the compiler has looked at the static function and realised that is doesn't use r8 so it need not be saved even though it is a volatile register.
In this code the compiler knows %ax is being used, it just doesn't know it is changed - so could assume the value is unchanged.
The only code that is likely to break is:
int f(int d) { d += 10; __delay(d); return d; }
Which might manage to return the value of %eax modified by the asm.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Ammar Faizi ammarfaizi2@gnuweeb.org
@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/20200403161943.1458-6-bp@alien8.de [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 bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: "H. Peter Anvin" hpa@zytor.com Cc: Ingo Molnar mingo@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Tony Luck tony.luck@intel.com Cc: stable@vger.kernel.org # v5.8+ Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path") Co-authored-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org --- arch/x86/kernel/cpu/mce/amd.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
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; }
On Tue, Mar 01, 2022 at 04:46:08PM +0700, Ammar Faizi wrote:
From: Ammar Faizi ammarfaizi2@gnuweeb.org
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; }
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...
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:
- _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 ammarfaizi2@gnuweeb.org 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 bp@alien8.de Cc: Thomas Gleixner tglx@linutronix.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org # v5.8+ Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path") Co-authored-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Co-authored-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org --- 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; }
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
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...
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 ammarfaizi2@gnuweeb.org 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 bp@alien8.de Cc: Thomas Gleixner tglx@linutronix.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org # v5.8+ Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path") Co-authored-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Co-authored-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org --- 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; }
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
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 ammarfaizi2@gnuweeb.org 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 bp@alien8.de Cc: Thomas Gleixner tglx@linutronix.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org # v5.8+ Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path") Co-authored-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Signed-off-by: Alviro Iskandar Setiawan alviro.iskandar@gnuweeb.org Co-authored-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Yazen Ghannam yazen.ghannam@amd.com Signed-off-by: Ammar Faizi ammarfaizi2@gnuweeb.org
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/9dfe087a-f941-1bc4-657d-7e7c198888ff@gnuweeb.or...
Thanks!
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 ammarfaizi2@gnuweeb.org 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
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/20220310015306.445359-1-ammarfaizi2@gnuweeb.org...
linux-stable-mirror@lists.linaro.org
-
Alviro Iskandar Setiawan -
Ammar Faizi -
David Laight -
Yazen Ghannam