Hi,
This is the v6 of two x86 fixes.
1) x86/delay: Fix the wrong Assembly constraint in delay_loop() function.
2) x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails.
## Changelog
v6:
- Remove unnecessary Cc tags.
- Undo the stable mark for patch 1.
- Update commit message, emphasize the danger when the compiler
decides to inline the function.
- Fix the Fixes tag sha1 in patch 1.
- Change the helper function name to __threshold_remove_device().
v5:
- Mark patch #1 for stable.
- Commit message improvement for patch #1 and #2.
- Fold in changes from Yazen and Alviro (for patch #2).
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.
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
Signed-off-by: Yazen Ghannam <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
---
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 | 32 +++++++++++++++++++-------------
arch/x86/lib/delay.c | 4 ++--
2 files changed, 21 insertions(+), 15 deletions(-)
base-commit: 1930a6e739c4b4a654a69164dbe39e554d228915
--
Ammar Faizi
In mce_threshold_create_device(), if threshold_create_bank() fails, the
@bp will be leaked, because the call to mce_threshold_remove_device()
will not free the @bp. mce_threshold_remove_device() frees
@threshold_banks. At that point, the @bp has not been written to
@threshold_banks, @threshold_banks is NULL, so the call is just a nop.
Fix this by extracting the cleanup part into a new static function
__threshold_remove_device(), then call it from create/remove device
functions.
Also, eliminate the "goto out_err", just early return inside the loop
if the creation fails.
Cc: [email protected] # v5.8+
Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
Co-developed-by: Alviro Iskandar Setiawan <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
Co-developed-by: Yazen Ghannam <[email protected]>
Signed-off-by: Yazen Ghannam <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
---
v6:
- Change the helper function name to __threshold_remove_device().
---
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 1940d305db1c..d293ae088d6b 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1294,10 +1294,23 @@ static void threshold_remove_bank(struct threshold_bank *bank)
kfree(bank);
}
+static void __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;
@@ -1308,13 +1321,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);
+ __threshold_remove_device(bp, this_cpu_read(mce_num_banks));
return 0;
}
@@ -1351,15 +1358,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) {
+ __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;
}
--
Ammar Faizi
On 4/4/22 12:43 AM, Ammar Faizi wrote:
> On 4/4/22 12:03 AM, Thomas Gleixner wrote:
>> On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote:
>>
>>> In mce_threshold_create_device(), if threshold_create_bank() fails, the
>>> @bp will be leaked, because the call to mce_threshold_remove_device()
>>> will not free the @bp. mce_threshold_remove_device() frees
>>> @threshold_banks. At that point, the @bp has not been written to
>>> @threshold_banks, @threshold_banks is NULL, so the call is just a nop.
>>>
>>> Fix this by extracting the cleanup part into a new static function
>>> __threshold_remove_device(), then call it from create/remove device
>>> functions.
>>
>> The way simpler fix is to move
>>
>>> }
>>> this_cpu_write(threshold_banks, bp);
>>
>> before the loop. That's safe because the banks cannot yet be reached via
>> an MCE as the vector is not yet enabled:
>>> if (thresholding_irq_en)
>>> mce_threshold_vector = amd_threshold_interrupt;
> Thomas,
>
> I did like what you said (in the patch v4), but after Yazen and Borislav
> reviewed it, we got a conclusion that it's not safe.
>
> See [1] and [2] for the full message.
>
> [1]: https://lore.kernel.org/lkml/[email protected]/
> [2]: https://lore.kernel.org/lkml/Yh+oyD%2F5M3TW5ZMM@yaz-ubuntu/
>
> Yazen, Borislav, please take a deeper look on this again. I will send
> a v7 revision to really make it simpler by moving that "per-CPU var write"
> before the loop.
(only if it's really safe)
--
Ammar Faizi
On 4/4/22 12:03 AM, Thomas Gleixner wrote:
> On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote:
>
>> In mce_threshold_create_device(), if threshold_create_bank() fails, the
>> @bp will be leaked, because the call to mce_threshold_remove_device()
>> will not free the @bp. mce_threshold_remove_device() frees
>> @threshold_banks. At that point, the @bp has not been written to
>> @threshold_banks, @threshold_banks is NULL, so the call is just a nop.
>>
>> Fix this by extracting the cleanup part into a new static function
>> __threshold_remove_device(), then call it from create/remove device
>> functions.
>
> The way simpler fix is to move
>
>> }
>> this_cpu_write(threshold_banks, bp);
>
> before the loop. That's safe because the banks cannot yet be reached via
> an MCE as the vector is not yet enabled:
>
>> if (thresholding_irq_en)
>> mce_threshold_vector = amd_threshold_interrupt;
Thomas,
I did like what you said (in the patch v4), but after Yazen and Borislav
reviewed it, we got a conclusion that it's not safe.
See [1] and [2] for the full message.
[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: https://lore.kernel.org/lkml/Yh+oyD%2F5M3TW5ZMM@yaz-ubuntu/
Yazen, Borislav, please take a deeper look on this again. I will send
a v7 revision to really make it simpler by moving that "per-CPU var write"
before the loop.
Thanks!
--
Ammar Faizi
On Mon, Apr 04, 2022 at 12:45:04AM +0700, Ammar Faizi wrote:
> > Yazen, Borislav, please take a deeper look on this again. I will send
> > a v7 revision to really make it simpler by moving that "per-CPU var write"
> > before the loop.
>
> (only if it's really safe)
Don't bother - I've discussed it with tglx offlist. The current solution
remains.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote:
> In mce_threshold_create_device(), if threshold_create_bank() fails, the
> @bp will be leaked, because the call to mce_threshold_remove_device()
> will not free the @bp. mce_threshold_remove_device() frees
> @threshold_banks. At that point, the @bp has not been written to
> @threshold_banks, @threshold_banks is NULL, so the call is just a nop.
>
> Fix this by extracting the cleanup part into a new static function
> __threshold_remove_device(), then call it from create/remove device
> functions.
The way simpler fix is to move
> }
> this_cpu_write(threshold_banks, bp);
before the loop. That's safe because the banks cannot yet be reached via
an MCE as the vector is not yet enabled:
> if (thresholding_irq_en)
> mce_threshold_vector = amd_threshold_interrupt;
Thanks,
tglx
The following commit has been merged into the ras/core branch of tip:
Commit-ID: e5f28623ceb103e13fc3d7bd45edf9818b227fd0
Gitweb: https://git.kernel.org/tip/e5f28623ceb103e13fc3d7bd45edf9818b227fd0
Author: Ammar Faizi <[email protected]>
AuthorDate: Tue, 29 Mar 2022 17:47:05 +07:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 05 Apr 2022 21:24:37 +02:00
x86/MCE/AMD: Fix memory leak when threshold_create_bank() fails
In mce_threshold_create_device(), if threshold_create_bank() fails, the
previously allocated threshold banks array @bp will be leaked because
the call to mce_threshold_remove_device() will not free it.
This happens because mce_threshold_remove_device() fetches the pointer
through the threshold_banks per-CPU variable but bp is written there
only after the bank creation is successful, and not before, when
threshold_create_bank() fails.
Add a helper which unwinds all the bank creation work previously done
and pass into it the previously allocated threshold banks array for
freeing.
[ bp: Massage. ]
Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
Co-developed-by: Alviro Iskandar Setiawan <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
Co-developed-by: Yazen Ghannam <[email protected]>
Signed-off-by: Yazen Ghannam <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 1940d30..1c87501 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1294,10 +1294,23 @@ out_free:
kfree(bank);
}
+static void __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])
+ continue;
+
+ 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 +1321,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);
+ __threshold_remove_device(bp);
return 0;
}
@@ -1351,15 +1358,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) {
+ __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;
}