lkml.org 
[lkml]   [2022]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] x86/MCE/AMD: Decrement threshold_bank refcount when removing threshold blocks
On Wed, Jun 15, 2022 at 01:51:47PM +0000, Yazen Ghannam wrote:
> Yes, I believe that's true based on code inspection. But I'm not aware of any
> reported issues in this area before the commit listed above. So I decided to
> switch the Fixes tag from what I had before (shown below). I can switch it
> back if you think that's best.
>
> Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")

Yeah, that is probably the culprit. I finally got to this and am able to
repro on my F10h box.

Here's what I think the fix should be, Greg, please check this
for no-nos, especially for doing a kobject_put() on the parent in
remove_shared_bank_kobjects(). But that is basically the reverse
operation of the kobject_add() I'm doing when sharing the bank, more to
that below.

I wonder why we see this now - maybe the kobject reference counting got
changed since then...

Anyway, thoughts?

---
From: Borislav Petkov <bp@suse.de>

x86/MCE/AMD: Correctly drop shared bank references

Old AMD machines have a shared MCA bank 4 which reports northbridge
error types. That bank has a bunch of controls which are exposed this
way in sysfs on CPU0:

/sys/devices/system/machinecheck/machinecheck0/northbridge/
├── dram
│   ├── error_count
│   ├── interrupt_enable
│   └── threshold_limit
├── ht_links
│   ├── error_count
│   ├── interrupt_enable
│   └── threshold_limit
└── l3_cache
├── error_count
├── interrupt_enable
└── threshold_limit

In order to expose the exact same controls - the bank is shared
between all CPUs - threshold_create_bank() reuses the bank pointer and
kobject_add()s it to the parent of the other CPUs:

mce: threshold_create_bank: CPU1, yes, use it, kref: 4, parent_kref: 3, name: northbridge
mce: threshold_create_bank: CPU1, inc cpus: 2, bank ref: 4
mce: __threshold_add_blocks: entry, kobj: 0xffff888100adb218, parent: 0xffff888100c10c00 ref: 1, parent_kref: 4, name: dram
mce: __threshold_add_blocks: misc, kobj: 0xffff888100adb418, parent: 0xffff888100c10c00, kref: 1, parent_kref: 6, name: l3_cache
mce: __threshold_add_blocks: misc, kobj: 0xffff888100adb318, parent: 0xffff888100c10c00, kref: 1, parent_kref: 7, name: ht_links
...

kobject_add() does a kobject_get() on the parent for each sysfs file it
adds.

Therefore, in order to unwind the same setup work when the CPU goes
offline and the bank *references* only are being removed - the other
CPUs still share it - do a kobject_put() on the parent.

Rename things more properly while at it and add comments.

Signed-off-by: Borislav Petkov <bp@suse.de>

---

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1c87501e0fa3..b2bdee9e0bae 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1241,31 +1241,40 @@ static void threshold_block_release(struct kobject *kobj)
kfree(to_block(kobj));
}

+/*
+ * Drop refcounts and delete list heads in order to free the memory.
+ */
static void deallocate_threshold_blocks(struct threshold_bank *bank)
{
+ struct list_head *head = &bank->blocks->miscj;
struct threshold_block *pos, *tmp;

- list_for_each_entry_safe(pos, tmp, &bank->blocks->miscj, miscj) {
- list_del(&pos->miscj);
+ list_for_each_entry_safe(pos, tmp, head, miscj) {
kobject_put(&pos->kobj);
+ list_del(&pos->miscj);
}

kobject_put(&bank->blocks->kobj);
}

-static void __threshold_remove_blocks(struct threshold_bank *b)
+/*
+ * Only put the parent kobject of each block. The inverse of kobject_add()
+ * above in threshold_create_bank().
+ */
+static void remove_shared_bank_kobjects(struct threshold_bank *bank)
{
- struct threshold_block *pos = NULL;
- struct threshold_block *tmp = NULL;
+ struct list_head *head = &bank->blocks->miscj;
+ struct threshold_block *pos, *tmp;

- kobject_del(b->kobj);
+ list_for_each_entry_safe(pos, tmp, head, miscj)
+ kobject_put(pos->kobj.parent);

- list_for_each_entry_safe(pos, tmp, &b->blocks->miscj, miscj)
- kobject_del(&pos->kobj);
+ kobject_put(bank->kobj);
}

static void threshold_remove_bank(struct threshold_bank *bank)
{
+ int cpu = smp_processor_id();
struct amd_northbridge *nb;

if (!bank->blocks)
@@ -1275,14 +1284,14 @@ static void threshold_remove_bank(struct threshold_bank *bank)
goto out_dealloc;

if (!refcount_dec_and_test(&bank->cpus)) {
- __threshold_remove_blocks(bank);
+ remove_shared_bank_kobjects(bank);
return;
} else {
/*
* The last CPU on this node using the shared bank is going
* away, remove that bank now.
*/
- nb = node_to_amd_nb(topology_die_id(smp_processor_id()));
+ nb = node_to_amd_nb(topology_die_id(cpu));
nb->bank4 = NULL;
}

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

\
 
 \ /
  Last update: 2022-10-26 12:17    [W:0.222 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site