Messages in this thread Patch in this message | | | Date | Wed, 26 Oct 2022 12:16:40 +0200 | From | Borislav Petkov <> | Subject | Re: [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
| |