x86/MCE/AMD: Redo function to get SMCA bank type

PATCH Superseded Yazen Ghannam 5 participants 24 messages Feb 1, 2018 – May 19, 2018

Subsystems (3)

x86 EDAC-core EDAC-AMD64

lore

Share & tools

lore

Yazen Ghannam

author maintainer

2018-02-01 18:48 UTC #0 lore raw

PATCH 1/3 x86/MCE/AMD: Redo function to get SMCA bank type

From: Yazen Ghannam <[email protected]>

Pass the bank number to smca_get_bank_type() since that's all we need.

Also, we should compare the bank number to the size of the smca_banks array not the number of bank types. Bank types are reused for multiple banks, so the number of types can be different from the number of banks in a system. We could return an invalid bank type.

Use smca_get_bank_type() in get_name(), and change type of bank_type variable to match return type of smca_get_bank_type().

Cc <[email protected]> # 4.14.x: 11cf887728a3 x86/MCE/AMD: Define a function to get SMCA bank type

Cc <[email protected]> # 4.14.x: c6708d50f166 x86/MCE: Report only DRAM ECC as memory errors on AMD systems

Cc <[email protected]> # 4.14.x

Signed-off-by Yazen Ghannam <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +7 -8

index 0f32ad242324..4e16afc0794d 100644
@@ -110,14 +110,14 @@ const char *smca_get_long_name(enum smca_bank_types t)
 }
 EXPORT_SYMBOL_GPL(smca_get_long_name);
 
-static enum smca_bank_types smca_get_bank_type(struct mce *m)
+static enum smca_bank_types smca_get_bank_type(unsigned int bank)
 {
 	struct smca_bank *b;
 
-	if (m->bank >= N_SMCA_BANK_TYPES)
+	if (bank >= ARRAY_SIZE(smca_banks))
 		return N_SMCA_BANK_TYPES;
 
-	b = &smca_banks[m->bank];
+	b = &smca_banks[bank];
 	if (!b->hwid)
 		return N_SMCA_BANK_TYPES;

@@ -760,7 +760,7 @@ bool amd_mce_is_memory_error(struct mce *m)
 	u8 xec = (m->status >> 16) & 0x1f;
 
 	if (mce_flags.smca)
-		return smca_get_bank_type(m) == SMCA_UMC && xec == 0x0;
+		return smca_get_bank_type(m->bank) == SMCA_UMC && xec == 0x0;
 
 	return m->bank == 4 && xec == 0x8;
 }

@@ -1063,7 +1063,7 @@ static struct kobj_type threshold_ktype = {
 
 static const char *get_name(unsigned int bank, struct threshold_block *b)
 {
-	unsigned int bank_type;
+	enum smca_bank_types bank_type;
 
 	if (!mce_flags.smca) {
 		if (b && bank == 4)

@@ -1072,11 +1072,10 @@ static const char *get_name(unsigned int bank, struct threshold_block *b)
 		return th_names[bank];
 	}
 
-	if (!smca_banks[bank].hwid)
+	bank_type = smca_get_bank_type(bank);
+	if (bank_type >= N_SMCA_BANK_TYPES)
 		return NULL;
 
-	bank_type = smca_banks[bank].hwid->bank_type;
-
 	if (b && bank_type == SMCA_UMC) {
 		if (b->block < ARRAY_SIZE(smca_umc_block_names))
 			return smca_umc_block_names[b->block];

Yazen Ghannam

author maintainer

2018-02-01 18:48 UTC #1 lore raw

PATCH 2/3 x86/MCE/AMD, EDAC/mce_amd: Enumerate Reserved SMCA bank type

From: Yazen Ghannam <[email protected]>

Currently, bank 4 is reserved on Fam17h, so we chose not to initialize bank 4 in the smca_banks array. This means that when we check if a bank is initialized, like during boot or resume, we will see that bank 4 is not initialized and try to initialize it. This may cause a call trace, when resuming from suspend, due to *on_cpu() calls in the init path.

Reserved banks will be read-as-zero, so their MCA_IPID register will be zero. So, like the smca_banks array, the threshold_banks array will not have an entry for a reserved bank since all its MCA_MISC* registers will be zero.

Enumerate a "Reserved" bank type that matches on a HWID_MCATYPE of 0,0.

Use the "Reserved" type when checking if a bank is reserved. It's possible that other bank numbers may be reserved on future systems.

Don't try to find the block address on reserved banks.

Cc <[email protected]> # 4.14.x

Signed-off-by Yazen Ghannam <[email protected]>

---
 arch/x86/include/asm/mce.h           |  1 +
 arch/x86/kernel/cpu/mcheck/mce_amd.c |  7 +++++++
 drivers/edac/mce_amd.c               | 11 +++++++----
 3 files changed, 15 insertions(+), 4 deletions(-)

arch/x86/include/asm/mce.h +1 -0

index 96ea4b5ba658..340070415c2c 100644
@@ -346,6 +346,7 @@ enum smca_bank_types {
 	SMCA_IF,	/* Instruction Fetch */
 	SMCA_L2_CACHE,	/* L2 Cache */
 	SMCA_DE,	/* Decoder Unit */
+	SMCA_RESERVED,	/* Reserved */
 	SMCA_EX,	/* Execution Unit */
 	SMCA_FP,	/* Floating Point */
 	SMCA_L3_CACHE,	/* L3 Cache */

arch/x86/kernel/cpu/mcheck/mce_amd.c +7 -0

index 4e16afc0794d..bf53b4549a17 100644
@@ -82,6 +82,7 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_IF]	= { "insn_fetch",	"Instruction Fetch Unit" },
 	[SMCA_L2_CACHE]	= { "l2_cache",		"L2 Cache" },
 	[SMCA_DE]	= { "decode_unit",	"Decode Unit" },
+	[SMCA_RESERVED]	= { "reserved",		"Reserved" },
 	[SMCA_EX]	= { "execution_unit",	"Execution Unit" },
 	[SMCA_FP]	= { "floating_point",	"Floating Point Unit" },
 	[SMCA_L3_CACHE]	= { "l3_cache",		"L3 Cache" },

@@ -127,6 +128,9 @@ static enum smca_bank_types smca_get_bank_type(unsigned int bank)
 static struct smca_hwid smca_hwid_mcatypes[] = {
 	/* { bank_type, hwid_mcatype, xec_bitmap } */
 
+	/* Reserved type */
+	{ SMCA_RESERVED, HWID_MCATYPE(0x00, 0x0), 0x0 },
+
 	/* ZN Core (HWID=0xB0) MCA types */
 	{ SMCA_LS,	 HWID_MCATYPE(0xB0, 0x0), 0x1FFFEF },
 	{ SMCA_IF,	 HWID_MCATYPE(0xB0, 0x1), 0x3FFF },

@@ -433,6 +437,9 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 	u32 addr = 0, offset = 0;
 
 	if (mce_flags.smca) {
+		if (smca_get_bank_type(bank) == SMCA_RESERVED)
+			return addr;
+
 		if (!block) {
 			addr = MSR_AMD64_SMCA_MCx_MISC(bank);
 		} else {

drivers/edac/mce_amd.c +7 -4

index a11a671c7a38..2ab4d61ee47e 100644
@@ -854,21 +854,24 @@ static void decode_mc6_mce(struct mce *m)
 static void decode_smca_error(struct mce *m)
 {
 	struct smca_hwid *hwid;
-	unsigned int bank_type;
+	enum smca_bank_types bank_type;
 	const char *ip_name;
 	u8 xec = XEC(m->status, xec_mask);
 
 	if (m->bank >= ARRAY_SIZE(smca_banks))
 		return;
 
-	if (x86_family(m->cpuid) >= 0x17 && m->bank == 4)
-		pr_emerg(HW_ERR "Bank 4 is reserved on Fam17h.\n");
-
 	hwid = smca_banks[m->bank].hwid;
 	if (!hwid)
 		return;
 
 	bank_type = hwid->bank_type;
+
+	if (bank_type == SMCA_RESERVED) {
+		pr_emerg(HW_ERR "Bank %d is reserved.\n", m->bank);
+		return;
+	}
+
 	ip_name = smca_get_long_name(bank_type);
 
 	pr_emerg(HW_ERR "%s Extended Error Code: %d\n", ip_name, xec);

Borislav Petkov

maintainer

2018-02-08 15:15 UTC #2 lore raw

Re: [PATCH 2/3] x86/MCE/AMD, EDAC/mce_amd: Enumerate Reserved SMCA bank type

Quoted 7 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
Currently, bank 4 is reserved on Fam17h, so we chose not to initialize bank 4 in the smca_banks array. This means that when we check if a bank is initialized, like during boot or resume, we will see that bank 4 is not initialized and try to initialize it. This may cause a call trace, when resuming from suspend, due to *on_cpu() calls in the init path.

Please be more specific: the rdmsr_*_on_cpu() calls issue an IPI but we're running with interrupts disabled, which triggers:

 WARNING: CPU: 0 PID: 11523 at kernel/smp.c:291 smp_call_function_single+0xdc/0xe0

Quoted 55 lines

Enumerate a "Reserved" bank type that matches on a HWID_MCATYPE of 0,0.

Use the "Reserved" type when checking if a bank is reserved. It's possible that other bank numbers may be reserved on future systems.

Don't try to find the block address on reserved banks.

Cc <[email protected]> # 4.14.x

Signed-off-by Yazen Ghannam <[email protected]>

---
 arch/x86/include/asm/mce.h           |  1 +
 arch/x86/kernel/cpu/mcheck/mce_amd.c |  7 +++++++
 drivers/edac/mce_amd.c               | 11 +++++++----
 3 files changed, 15 insertions(+), 4 deletions(-)

arch/x86/include/asm/mce.h +1 -0

index 96ea4b5ba658..340070415c2c 100644
@@ -346,6 +346,7 @@ enum smca_bank_types {
 	SMCA_IF,	/* Instruction Fetch */
 	SMCA_L2_CACHE,	/* L2 Cache */
 	SMCA_DE,	/* Decoder Unit */
+	SMCA_RESERVED,	/* Reserved */
 	SMCA_EX,	/* Execution Unit */
 	SMCA_FP,	/* Floating Point */
 	SMCA_L3_CACHE,	/* L3 Cache */

arch/x86/kernel/cpu/mcheck/mce_amd.c +4 -0

index 4e16afc0794d..bf53b4549a17 100644
@@ -82,6 +82,7 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_IF]	= { "insn_fetch",	"Instruction Fetch Unit" },
 	[SMCA_L2_CACHE]	= { "l2_cache",		"L2 Cache" },
 	[SMCA_DE]	= { "decode_unit",	"Decode Unit" },
+	[SMCA_RESERVED]	= { "reserved",		"Reserved" },
 	[SMCA_EX]	= { "execution_unit",	"Execution Unit" },
 	[SMCA_FP]	= { "floating_point",	"Floating Point Unit" },
 	[SMCA_L3_CACHE]	= { "l3_cache",		"L3 Cache" },

@@ -127,6 +128,9 @@ static enum smca_bank_types smca_get_bank_type(unsigned int bank)
 static struct smca_hwid smca_hwid_mcatypes[] = {
 	/* { bank_type, hwid_mcatype, xec_bitmap } */
 
+	/* Reserved type */
+	{ SMCA_RESERVED, HWID_MCATYPE(0x00, 0x0), 0x0 },
+
 	/* ZN Core (HWID=0xB0) MCA types */
 	{ SMCA_LS,	 HWID_MCATYPE(0xB0, 0x0), 0x1FFFEF },
 	{ SMCA_IF,	 HWID_MCATYPE(0xB0, 0x1), 0x3FFF },

arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi · line 439

Quoted 3 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0

@@ -433,6 +437,9 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 	u32 addr = 0, offset = 0;
 
 	if (mce_flags.smca) {

As a last patch in the series: please carve the code in this if-statement into a smca_get_block_address() helper. And it doesn't need the stable tag as it is only a cleanup.

Thx.

Ghannam, Yazen

author maintainer

2018-02-14 16:28 UTC #3 lore raw

RE: [PATCH 2/3] x86/MCE/AMD, EDAC/mce_amd: Enumerate Reserved SMCA bank type

Quoted 19 lines

-----Original Message-----
From: [email protected] [mailto:linux-edac- [email protected]] On Behalf Of Borislav Petkov
Sent: Thursday, February 8, 2018 10:15 AM
To: Ghannam, Yazen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] x86/MCE/AMD, EDAC/mce_amd: Enumerate Reserved SMCA bank type
Quoted 7 lines · Yazen Ghannam
From: Yazen Ghannam <[email protected]>
Currently, bank 4 is reserved on Fam17h, so we chose not to initialize bank 4 in the smca_banks array. This means that when we check if a bank is initialized, like during boot or resume, we will see that bank 4 is not initialized and try to initialize it. This may cause a call trace, when resuming from suspend, due to *on_cpu() calls in the init path.

Quoted 6 lines

Please be more specific: the rdmsr_*_on_cpu() calls issue an IPI but we're running with interrupts disabled, which triggers:
 WARNING: CPU: 0 PID: 11523 at kernel/smp.c:291
smp_call_function_single+0xdc/0xe0

Okay.

...

Quoted 10 lines

Review static u32 get_block_address(unsigned int cpu, u32
Quoted 1 line
(quoted diff) +0 -0
@@ -433,6 +437,9 @@ static u32 get_block_address(unsigned int cpu, u32
current_addr, u32 low, u32 hi
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi · line 439
Quoted 3 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
 	u32 addr = 0, offset = 0;
 
 	if (mce_flags.smca) {
As a last patch in the series: please carve the code in this if-statement into a smca_get_block_address() helper. And it doesn't need the stable tag as it is only a cleanup.

Will do.

Thanks,
Yazen

Yazen Ghannam

author maintainer

2018-02-01 18:48 UTC #4 lore raw

PATCH 3/3 x86/MCE/AMD: Get address from already initialized block

In reply to Yazen Ghannam

From: Yazen Ghannam <[email protected]>

The block address is saved after the block is initialized when threshold_init_device() is called.

Use the saved block address, if available, rather than trying to rediscover it.

We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.

Cc <[email protected]> # 4.14.x

Signed-off-by Yazen Ghannam <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

arch/x86/kernel/cpu/mcheck/mce_amd.c +15 -0

index bf53b4549a17..8c4f8f30c779 100644
@@ -436,6 +436,21 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 {
 	u32 addr = 0, offset = 0;
 
+	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+		return addr;
+
+	/* Get address from already initialized block. */
+	if (per_cpu(threshold_banks, cpu)) {
+		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
+
+		if (bankp && bankp->blocks) {
+			struct threshold_block *blockp = &bankp->blocks[block];
+
+			if (blockp)
+				return blockp->address;
+		}
+	}
+
 	if (mce_flags.smca) {
 		if (smca_get_bank_type(bank) == SMCA_RESERVED)
 			return addr;

Borislav Petkov

maintainer

2018-02-08 15:26 UTC #5 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 10 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
The block address is saved after the block is initialized when threshold_init_device() is called.
Use the saved block address, if available, rather than trying to rediscover it.
We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.

Ditto.

Johannes Hirte

2018-04-14 00:42 UTC #6 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

In reply to Yazen Ghannam

Quoted 45 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
The block address is saved after the block is initialized when threshold_init_device() is called.
Use the saved block address, if available, rather than trying to rediscover it.
We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.
Cc <[email protected]> # 4.14.x
Signed-off-by Yazen Ghannam <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
arch/x86/kernel/cpu/mcheck/mce_amd.c +15 -0
index bf53b4549a17..8c4f8f30c779 100644
@@ -436,6 +436,21 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 {
 	u32 addr = 0, offset = 0;
 
+	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+		return addr;
+
+	/* Get address from already initialized block. */
+	if (per_cpu(threshold_banks, cpu)) {
+		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
+
+		if (bankp && bankp->blocks) {
+			struct threshold_block *blockp = &bankp->blocks[block];
+
+			if (blockp)
+				return blockp->address;
+		}
+	}
+
 	if (mce_flags.smca) {
 		if (smca_get_bank_type(bank) == SMCA_RESERVED)
 			return addr;
Signature · 1 line
2.14.1

I have a KASAN: slab-out-of-bounds, and git bisect points me to this change:

Apr 13 00:40:32 probook kernel: ==================================================================
Apr 13 00:40:32 probook kernel: BUG: KASAN: slab-out-of-bounds in get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel: Read of size 4 at addr ffff8803f165ddf4 by task swapper/0/1
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.16.0-10757-g4ca8ba4ccff9 #532
Apr 13 00:40:32 probook kernel: Hardware name: HP HP ProBook 645 G2/80FE, BIOS N77 Ver. 01.12 12/19/2017
Apr 13 00:40:32 probook kernel: Call Trace:
Apr 13 00:40:32 probook kernel:  dump_stack+0x5b/0x8b
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  print_address_description+0x65/0x270
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  kasan_report+0x232/0x350
Apr 13 00:40:32 probook kernel:  get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  ? kobject_init_and_add+0xde/0x130
Apr 13 00:40:32 probook kernel:  ? get_name+0x390/0x390
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x12c/0xc60
Apr 13 00:40:32 probook kernel:  ? kobject_add_internal+0x800/0x800
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x520/0x520
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  mce_threshold_create_device+0x35b/0x990
Apr 13 00:40:32 probook kernel:  ? init_special_inode+0x1d0/0x230
Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Apr 13 00:40:32 probook kernel:  ? mcheck_vendor_init_severity+0x43/0x43
Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Apr 13 00:40:32 probook kernel:  ? trace_event_raw_event_initcall_finish+0x190/0x190
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0xb/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Allocated by task 1:
Apr 13 00:40:32 probook kernel:  kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  kmem_cache_alloc_trace+0xf3/0x1f0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x1bc/0xc60
Apr 13 00:40:32 probook kernel:  mce_threshold_create_device+0x35b/0x990
Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: Freed by task 0:
Apr 13 00:40:32 probook kernel: (stack is not available)
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: The buggy address belongs to the object at ffff8803f165dd80
 which belongs to the cache kmalloc-128 of size 128
Apr 13 00:40:32 probook kernel: The buggy address is located 116 bytes inside of
 128-byte region [ffff8803f165dd80, ffff8803f165de00)
 Apr 13 00:40:32 probook kernel: The buggy address belongs to the page:
Apr 13 00:40:32 probook kernel: page:ffffea000fc59740 count:1 mapcount:0 mapping:0000000000000000 index:0x0
Apr 13 00:40:32 probook kernel: flags: 0x2000000000000100(slab)
Apr 13 00:40:32 probook kernel: raw: 2000000000000100 0000000000000000 0000000000000000 0000000180150015
Apr 13 00:40:32 probook kernel: raw: dead000000000100 dead000000000200 ffff8803f3403340 0000000000000000
Apr 13 00:40:32 probook kernel: page dumped because: kasan: bad access detected
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: Memory state around the buggy address:
Apr 13 00:40:32 probook kernel:  ffff8803f165dc80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
Apr 13 00:40:32 probook kernel:  ffff8803f165dd00: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel: >ffff8803f165dd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
Apr 13 00:40:32 probook kernel:                                                              ^
Apr 13 00:40:32 probook kernel:  ffff8803f165de00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel:  ffff8803f165de80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel: ==================================================================

Johannes Hirte

2018-04-16 11:56 UTC #7 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 110 lines · Johannes Hirte

Quoted 45 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
The block address is saved after the block is initialized when threshold_init_device() is called.
Use the saved block address, if available, rather than trying to rediscover it.
We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.
Cc <[email protected]> # 4.14.x
Signed-off-by Yazen Ghannam <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
arch/x86/kernel/cpu/mcheck/mce_amd.c +15 -0
index bf53b4549a17..8c4f8f30c779 100644
@@ -436,6 +436,21 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 {
 	u32 addr = 0, offset = 0;
 
+	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+		return addr;
+
+	/* Get address from already initialized block. */
+	if (per_cpu(threshold_banks, cpu)) {
+		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
+
+		if (bankp && bankp->blocks) {
+			struct threshold_block *blockp = &bankp->blocks[block];
+
+			if (blockp)
+				return blockp->address;
+		}
+	}
+
 	if (mce_flags.smca) {
 		if (smca_get_bank_type(bank) == SMCA_RESERVED)
 			return addr;
Signature · 1 line
2.14.1

I have a KASAN: slab-out-of-bounds, and git bisect points me to this change:

Apr 13 00:40:32 probook kernel: ==================================================================
Apr 13 00:40:32 probook kernel: BUG: KASAN: slab-out-of-bounds in get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel: Read of size 4 at addr ffff8803f165ddf4 by task swapper/0/1
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.16.0-10757-g4ca8ba4ccff9 #532
Apr 13 00:40:32 probook kernel: Hardware name: HP HP ProBook 645 G2/80FE, BIOS N77 Ver. 01.12 12/19/2017
Apr 13 00:40:32 probook kernel: Call Trace:
Apr 13 00:40:32 probook kernel:  dump_stack+0x5b/0x8b
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  print_address_description+0x65/0x270
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  kasan_report+0x232/0x350
Apr 13 00:40:32 probook kernel:  get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  ? kobject_init_and_add+0xde/0x130
Apr 13 00:40:32 probook kernel:  ? get_name+0x390/0x390
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x12c/0xc60
Apr 13 00:40:32 probook kernel:  ? kobject_add_internal+0x800/0x800
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x520/0x520
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  mce_threshold_create_device+0x35b/0x990
Apr 13 00:40:32 probook kernel:  ? init_special_inode+0x1d0/0x230
Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Apr 13 00:40:32 probook kernel:  ? mcheck_vendor_init_severity+0x43/0x43
Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Apr 13 00:40:32 probook kernel:  ? trace_event_raw_event_initcall_finish+0x190/0x190
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0xb/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Allocated by task 1:
Apr 13 00:40:32 probook kernel:  kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  kmem_cache_alloc_trace+0xf3/0x1f0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x1bc/0xc60
Apr 13 00:40:32 probook kernel:  mce_threshold_create_device+0x35b/0x990
Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: Freed by task 0:
Apr 13 00:40:32 probook kernel: (stack is not available)
Apr 13 00:40:32 probook kernel: 
Apr 13 00:40:32 probook kernel: The buggy address belongs to the object at ffff8803f165dd80
 which belongs to the cache kmalloc-128 of size 128
Apr 13 00:40:32 probook kernel: The buggy address is located 116 bytes inside of
 128-byte region [ffff8803f165dd80, ffff8803f165de00)
 Apr 13 00:40:32 probook kernel: The buggy address belongs to the page:
Apr 13 00:40:32 probook kernel: page:ffffea000fc59740 count:1 mapcount:0 mapping:0000000000000000 index:0x0
Apr 13 00:40:32 probook kernel: flags: 0x2000000000000100(slab)
Apr 13 00:40:32 probook kernel: raw: 2000000000000100 0000000000000000 0000000000000000 0000000180150015
Apr 13 00:40:32 probook kernel: raw: dead000000000100 dead000000000200 ffff8803f3403340 0000000000000000
Apr 13 00:40:32 probook kernel: page dumped because: kasan: bad access detected
Apr 13 00:40:32 probook kernel:

Quoted 9 lines

Apr 13 00:40:32 probook kernel: Memory state around the buggy address:
Apr 13 00:40:32 probook kernel:  ffff8803f165dc80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
Apr 13 00:40:32 probook kernel:  ffff8803f165dd00: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel: >ffff8803f165dd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
Apr 13 00:40:32 probook kernel:                                                              ^
Apr 13 00:40:32 probook kernel:  ffff8803f165de00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel:  ffff8803f165de80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
Apr 13 00:40:32 probook kernel: ==================================================================

Putting the whole chaching part under the

if (mce_flags.smca) {

solved the issue on my Carrizo.

Ghannam, Yazen

author maintainer

2018-04-17 13:31 UTC #8 lore raw

RE: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 158 lines

-----Original Message-----
From: [email protected] <linux-edac- [email protected]> On Behalf Of Johannes Hirte
Sent: Monday, April 16, 2018 7:56 AM
To: Ghannam, Yazen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block
Quoted 12 lines · Johannes Hirte
Quoted 11 lines · Yazen Ghannam
From: Yazen Ghannam <[email protected]>
The block address is saved after the block is initialized when threshold_init_device() is called.
Use the saved block address, if available, rather than trying to rediscover it.
We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.
Quoted 7 lines
Review
Quoted 7 lines
Cc <[email protected]> # 4.14.x
Signed-off-by Yazen Ghannam <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
(quoted diff) +0 -0
b/arch/x86/kernel/cpu/mcheck/mce_amd.c
Quoted 4 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu,
Quoted 4 lines
index bf53b4549a17..8c4f8f30c779 100644
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
@@ -436,6 +436,21 @@ static u32 get_block_address(unsigned int cpu,
u32 current_addr, u32 low, u32 hi
Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +6 -0
 {
 	u32 addr = 0, offset = 0;
 
+	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+		return addr;
+
+	/* Get address from already initialized block. */
+	if (per_cpu(threshold_banks, cpu)) {
+		struct threshold_bank *bankp = per_cpu(threshold_banks,
cpu)[bank];
Quoted 19 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
Quoted 3 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +3 -0
+
+		if (bankp && bankp->blocks) {
+			struct threshold_block *blockp = &bankp-
blocks[block];
Quoted 11 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +6 -1
+
+			if (blockp)
+				return blockp->address;
+		}
+	}
+
 	if (mce_flags.smca) {
 		if (smca_get_bank_type(bank) == SMCA_RESERVED)
 			return addr;
--
2.14.1
I have a KASAN: slab-out-of-bounds, and git bisect points me to this change:
Quoted 1 line
Apr 13 00:40:32 probook kernel:
================================================================
==
Quoted 1 line
Apr 13 00:40:32 probook kernel: BUG: KASAN: slab-out-of-bounds in
get_block_address.isra.3+0x1e9/0x520
Quoted 1 line
Apr 13 00:40:32 probook kernel: Read of size 4 at addr ffff8803f165ddf4 by
task swapper/0/1
Quoted 2 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: CPU: 1 PID: 1 Comm: swapper/0 Not
tainted 4.16.0-10757-g4ca8ba4ccff9 #532
Quoted 1 line
Apr 13 00:40:32 probook kernel: Hardware name: HP HP ProBook 645
G2/80FE, BIOS N77 Ver. 01.12 12/19/2017
Quoted 8 lines
Apr 13 00:40:32 probook kernel: Call Trace:
Apr 13 00:40:32 probook kernel:  dump_stack+0x5b/0x8b
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  print_address_description+0x65/0x270
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  kasan_report+0x232/0x350
Apr 13 00:40:32 probook kernel:  get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  ? kobject_init_and_add+0xde/0x130
Quoted 8 lines
Apr 13 00:40:32 probook kernel:  ? get_name+0x390/0x390
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x12c/0xc60
Apr 13 00:40:32 probook kernel:  ? kobject_add_internal+0x800/0x800
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x520/0x520
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:
mce_threshold_create_device+0x35b/0x990
> Apr 13 00:40:32 probook kernel:  ? init_special_inode+0x1d0/0x230
> Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ?
mcheck_vendor_init_severity+0x43/0x43
> Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ?
trace_event_raw_event_initcall_finish+0x190/0x190
> Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0xb/0x40
> Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
> Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
> Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
> Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
> Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
> Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Quoted 6 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Allocated by task 1:
Apr 13 00:40:32 probook kernel:  kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  kmem_cache_alloc_trace+0xf3/0x1f0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x1bc/0xc60
Apr 13 00:40:32 probook kernel:
mce_threshold_create_device+0x35b/0x990
> Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
> Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
> Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
> Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
> Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Quoted 5 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Freed by task 0:
Apr 13 00:40:32 probook kernel: (stack is not available)
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: The buggy address belongs to the object at
ffff8803f165dd80
Quoted 2 lines
which belongs to the cache kmalloc-128 of size 128
Apr 13 00:40:32 probook kernel: The buggy address is located 116 bytes
inside of
Quoted 3 lines
128-byte region [ffff8803f165dd80, ffff8803f165de00)
 Apr 13 00:40:32 probook kernel: The buggy address belongs to the page:
Apr 13 00:40:32 probook kernel: page:ffffea000fc59740 count:1
mapcount:0 mapping:0000000000000000 index:0x0
Quoted 2 lines
Apr 13 00:40:32 probook kernel: flags: 0x2000000000000100(slab)
Apr 13 00:40:32 probook kernel: raw: 2000000000000100
0000000000000000 0000000000000000 0000000180150015
Quoted 1 line
Apr 13 00:40:32 probook kernel: raw: dead000000000100
dead000000000200 ffff8803f3403340 0000000000000000
Quoted 1 line
Apr 13 00:40:32 probook kernel: page dumped because: kasan: bad access
detected
Quoted 3 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Memory state around the buggy address:
Apr 13 00:40:32 probook kernel:  ffff8803f165dc80: fc fc fc fc fc fc fc fc 00 00
00 00 00 00 00 00
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ffff8803f165dd00: 00 00 00 00 00 00 00 fc
fc fc fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel: >ffff8803f165dd80: 00 00 00 00 00 00 00
00 00 00 00 00 00 fc fc fc
Quoted 2 lines
Apr 13 00:40:32 probook kernel:                                                              ^
Apr 13 00:40:32 probook kernel:  ffff8803f165de00: fc fc fc fc fc fc fc fc fc fc
fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ffff8803f165de80: fc fc fc fc fc fc fc fc fc fc
fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel:
================================================================
==
Quoted 1 line

Quoted 6 lines

Putting the whole chaching part under the
if (mce_flags.smca) {
solved the issue on my Carrizo.

Thanks for reporting this. I'm able to reproduce this on my Fam17h system. The caching should still be the same on non-SMCA systems. Putting it all under the SMCA flags effectively removes it on Carrizo.

Here are when get_block_address() is called:
1) Boot time MCE init. Called on each CPU. No caching.
2) Init of the MCE device. Called on a single CPU. Values are cached here.
3) CPU on/offling which calls MCE init. Should use the cached values.

It seems to me that the KASAN bug is detected during #2 though it's not yet clear to me what the issue is. I need to read up on KASAN and keep debugging.

Do any of the maintainers have any suggestions?

Thanks,
Yazen

Johannes Hirte

2018-05-15 09:39 UTC #9 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 169 lines · Yazen

Quoted 158 lines
-----Original Message-----
From: [email protected] <linux-edac- [email protected]> On Behalf Of Johannes Hirte
Sent: Monday, April 16, 2018 7:56 AM
To: Ghannam, Yazen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block
Quoted 12 lines · Johannes Hirte
Quoted 11 lines · Yazen Ghannam
From: Yazen Ghannam <[email protected]>
The block address is saved after the block is initialized when threshold_init_device() is called.
Use the saved block address, if available, rather than trying to rediscover it.
We can avoid some *on_cpu() calls in the init path that will cause a call trace when resuming from suspend.
Quoted 7 lines
Review
Quoted 7 lines
Cc <[email protected]> # 4.14.x
Signed-off-by Yazen Ghannam <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
(quoted diff) +0 -0
b/arch/x86/kernel/cpu/mcheck/mce_amd.c
Quoted 4 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu,
Quoted 4 lines
index bf53b4549a17..8c4f8f30c779 100644
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
@@ -436,6 +436,21 @@ static u32 get_block_address(unsigned int cpu,
u32 current_addr, u32 low, u32 hi
Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +6 -0
 {
 	u32 addr = 0, offset = 0;
 
+	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
+		return addr;
+
+	/* Get address from already initialized block. */
+	if (per_cpu(threshold_banks, cpu)) {
+		struct threshold_bank *bankp = per_cpu(threshold_banks,
cpu)[bank];
Quoted 19 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
Quoted 3 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +3 -0
+
+		if (bankp && bankp->blocks) {
+			struct threshold_block *blockp = &bankp-
blocks[block];
Quoted 11 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +6 -1
+
+			if (blockp)
+				return blockp->address;
+		}
+	}
+
 	if (mce_flags.smca) {
 		if (smca_get_bank_type(bank) == SMCA_RESERVED)
 			return addr;
--
2.14.1
I have a KASAN: slab-out-of-bounds, and git bisect points me to this change:
Quoted 1 line
Apr 13 00:40:32 probook kernel:
================================================================
==
Quoted 1 line
Apr 13 00:40:32 probook kernel: BUG: KASAN: slab-out-of-bounds in
get_block_address.isra.3+0x1e9/0x520
Quoted 1 line
Apr 13 00:40:32 probook kernel: Read of size 4 at addr ffff8803f165ddf4 by
task swapper/0/1
Quoted 2 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: CPU: 1 PID: 1 Comm: swapper/0 Not
tainted 4.16.0-10757-g4ca8ba4ccff9 #532
Quoted 1 line
Apr 13 00:40:32 probook kernel: Hardware name: HP HP ProBook 645
G2/80FE, BIOS N77 Ver. 01.12 12/19/2017
Quoted 8 lines
Apr 13 00:40:32 probook kernel: Call Trace:
Apr 13 00:40:32 probook kernel:  dump_stack+0x5b/0x8b
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  print_address_description+0x65/0x270
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  kasan_report+0x232/0x350
Apr 13 00:40:32 probook kernel:  get_block_address.isra.3+0x1e9/0x520
Apr 13 00:40:32 probook kernel:  ? kobject_init_and_add+0xde/0x130
Quoted 8 lines
Apr 13 00:40:32 probook kernel:  ? get_name+0x390/0x390
Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x12c/0xc60
Apr 13 00:40:32 probook kernel:  ? kobject_add_internal+0x800/0x800
Apr 13 00:40:32 probook kernel:  ? get_block_address.isra.3+0x520/0x520
Apr 13 00:40:32 probook kernel:  ? kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:
mce_threshold_create_device+0x35b/0x990
> Apr 13 00:40:32 probook kernel:  ? init_special_inode+0x1d0/0x230
> Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ?
mcheck_vendor_init_severity+0x43/0x43
> Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ?
trace_event_raw_event_initcall_finish+0x190/0x190
> Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0xb/0x40
> Apr 13 00:40:32 probook kernel:  ? kasan_unpoison_shadow+0x30/0x40
> Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
> Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
> Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
> Apr 13 00:40:32 probook kernel:  ? rest_init+0xf0/0xf0
> Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Quoted 6 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Allocated by task 1:
Apr 13 00:40:32 probook kernel:  kasan_kmalloc+0xa0/0xd0
Apr 13 00:40:32 probook kernel:  kmem_cache_alloc_trace+0xf3/0x1f0
Apr 13 00:40:32 probook kernel:  allocate_threshold_blocks+0x1bc/0xc60
Apr 13 00:40:32 probook kernel:
mce_threshold_create_device+0x35b/0x990
> Apr 13 00:40:32 probook kernel:  threshold_init_device+0x98/0xa7
> Apr 13 00:40:32 probook kernel:  do_one_initcall+0x76/0x30c
> Apr 13 00:40:32 probook kernel:  kernel_init_freeable+0x3d6/0x471
> Apr 13 00:40:32 probook kernel:  kernel_init+0xa/0x120
> Apr 13 00:40:32 probook kernel:  ret_from_fork+0x22/0x40
Quoted 5 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Freed by task 0:
Apr 13 00:40:32 probook kernel: (stack is not available)
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: The buggy address belongs to the object at
ffff8803f165dd80
Quoted 2 lines
which belongs to the cache kmalloc-128 of size 128
Apr 13 00:40:32 probook kernel: The buggy address is located 116 bytes
inside of
Quoted 3 lines
128-byte region [ffff8803f165dd80, ffff8803f165de00)
 Apr 13 00:40:32 probook kernel: The buggy address belongs to the page:
Apr 13 00:40:32 probook kernel: page:ffffea000fc59740 count:1
mapcount:0 mapping:0000000000000000 index:0x0
Quoted 2 lines
Apr 13 00:40:32 probook kernel: flags: 0x2000000000000100(slab)
Apr 13 00:40:32 probook kernel: raw: 2000000000000100
0000000000000000 0000000000000000 0000000180150015
Quoted 1 line
Apr 13 00:40:32 probook kernel: raw: dead000000000100
dead000000000200 ffff8803f3403340 0000000000000000
Quoted 1 line
Apr 13 00:40:32 probook kernel: page dumped because: kasan: bad access
detected
Quoted 3 lines
Apr 13 00:40:32 probook kernel:
Apr 13 00:40:32 probook kernel: Memory state around the buggy address:
Apr 13 00:40:32 probook kernel:  ffff8803f165dc80: fc fc fc fc fc fc fc fc 00 00
00 00 00 00 00 00
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ffff8803f165dd00: 00 00 00 00 00 00 00 fc
fc fc fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel: >ffff8803f165dd80: 00 00 00 00 00 00 00
00 00 00 00 00 00 fc fc fc
Quoted 2 lines
Apr 13 00:40:32 probook kernel:                                                              ^
Apr 13 00:40:32 probook kernel:  ffff8803f165de00: fc fc fc fc fc fc fc fc fc fc
fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel:  ffff8803f165de80: fc fc fc fc fc fc fc fc fc fc
fc fc fc fc fc fc
Quoted 1 line
Apr 13 00:40:32 probook kernel:
================================================================
==
Quoted 1 line
Quoted 6 lines
Putting the whole chaching part under the
if (mce_flags.smca) {
solved the issue on my Carrizo.
Thanks for reporting this. I'm able to reproduce this on my Fam17h system. The caching should still be the same on non-SMCA systems. Putting it all under the SMCA flags effectively removes it on Carrizo.

Quoted 7 lines

Here are when get_block_address() is called:
1) Boot time MCE init. Called on each CPU. No caching.
2) Init of the MCE device. Called on a single CPU. Values are cached here.
3) CPU on/offling which calls MCE init. Should use the cached values.
It seems to me that the KASAN bug is detected during #2 though it's not yet clear to me what the issue is. I need to read up on KASAN and keep debugging.

The out-of-bound access happens in get_block_address:

	if (bankp && bankp->blocks) {
		struct threshold_block *blockp blockp = &bankp->blocks[block];

with block=1. This doesn't exists. I don't even find any array here. There is a linked list, created in allocate_threshold_blocks. On my system I get 17 lists with one element each.

Borislav Petkov

2018-05-16 22:46 UTC #10 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 8 lines · Johannes Hirte

The out-of-bound access happens in get_block_address:
	if (bankp && bankp->blocks) {
		struct threshold_block *blockp blockp = &bankp->blocks[block];
with block=1. This doesn't exists. I don't even find any array here. There is a linked list, created in allocate_threshold_blocks. On my system I get 17 lists with one element each.

Yes, what a mess this is. ;-\

There's no such thing as ->blocks[block] array. We assign simply the threshold_block to it in allocate_threshold_blocks:

	per_cpu(threshold_banks, cpu)[bank]->blocks = b;

And I can't say the design of this thing is really friendly but it is still no excuse that I missed that during review. Grrr.

So, Yazen, what really needs to happen here is to iterate the bank->blocks->miscj list to find the block you're looking for and return its address, the opposite to this here:

        if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
                list_add(&b->miscj,
                         &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
        } else {
                per_cpu(threshold_banks, cpu)[bank]->blocks = b;
        }

and don't forget to look at ->blocks itself.

And then you need to make sure that searching for block addresses still works when resuming from suspend so that you can avoid the RDMSR IPIs.

Ok?

Johannes Hirte

2018-05-17 06:49 UTC #11 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 31 lines · Borislav Petkov

Quoted 8 lines · Johannes Hirte
The out-of-bound access happens in get_block_address:
	if (bankp && bankp->blocks) {
		struct threshold_block *blockp blockp = &bankp->blocks[block];
with block=1. This doesn't exists. I don't even find any array here. There is a linked list, created in allocate_threshold_blocks. On my system I get 17 lists with one element each.
Yes, what a mess this is. ;-\
There's no such thing as ->blocks[block] array. We assign simply the threshold_block to it in allocate_threshold_blocks:
	per_cpu(threshold_banks, cpu)[bank]->blocks = b;
And I can't say the design of this thing is really friendly but it is still no excuse that I missed that during review. Grrr.
So, Yazen, what really needs to happen here is to iterate the bank->blocks->miscj list to find the block you're looking for and return its address, the opposite to this here:
        if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
                list_add(&b->miscj,
                         &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
        } else {
                per_cpu(threshold_banks, cpu)[bank]->blocks = b;
        }

Quoted 5 lines

and don't forget to look at ->blocks itself.
And then you need to make sure that searching for block addresses still works when resuming from suspend so that you can avoid the RDMSR IPIs.

Maybe I'm missing something, but those RDMSR IPSs don't happen on pre-SMCA systems, right? So the caching should be avoided here, cause the whole lookup looks more expensive to me than the simple switch-block in get_block_address.

Borislav Petkov

2018-05-17 10:41 UTC #12 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 4 lines · Johannes Hirte

Maybe I'm missing something, but those RDMSR IPSs don't happen on pre-SMCA systems, right? So the caching should be avoided here, cause the whole lookup looks more expensive to me than the simple switch-block in get_block_address.

Yeah, and we should simply cache all those MSR values as I suggested then.

The patch at the end should fix your issue.

Yazen, so I'm working on the assumption that all addresses are the same on every core - I dumped them and it looks like this:

    128 bank: 00, block: 0 : 0xc0002003
    128 bank: 00, block: 1 : 0x0
    128 bank: 01, block: 0 : 0xc0002013
    128 bank: 01, block: 1 : 0x0
    128 bank: 02, block: 0 : 0xc0002023
    128 bank: 02, block: 1 : 0x0
    128 bank: 03, block: 0 : 0xc0002033
    128 bank: 03, block: 1 : 0x0
    128 bank: 04, block: 0 : 0x0
    128 bank: 05, block: 0 : 0xc0002053
    128 bank: 05, block: 1 : 0x0
    128 bank: 06, block: 0 : 0xc0002063
    128 bank: 06, block: 1 : 0x0
    128 bank: 07, block: 0 : 0xc0002073
    128 bank: 07, block: 1 : 0x0
    128 bank: 08, block: 0 : 0xc0002083
    128 bank: 08, block: 1 : 0x0
    128 bank: 09, block: 0 : 0xc0002093
    128 bank: 09, block: 1 : 0x0
    128 bank: 10, block: 0 : 0xc00020a3
    128 bank: 10, block: 1 : 0x0
    128 bank: 11, block: 0 : 0xc00020b3
    128 bank: 11, block: 1 : 0x0
    128 bank: 12, block: 0 : 0xc00020c3
    128 bank: 12, block: 1 : 0x0
    128 bank: 13, block: 0 : 0xc00020d3
    128 bank: 13, block: 1 : 0x0
    128 bank: 14, block: 0 : 0xc00020e3
    128 bank: 14, block: 1 : 0x0
    128 bank: 15, block: 0 : 0xc00020f3
    128 bank: 15, block: 1 : 0x0
    128 bank: 16, block: 0 : 0xc0002103
    128 bank: 16, block: 1 : 0x0
    128 bank: 17, block: 0 : 0xc0002113
    128 bank: 17, block: 1 : 0x0
    128 bank: 18, block: 0 : 0xc0002123
    128 bank: 18, block: 1 : 0x0
    128 bank: 19, block: 0 : 0xc0002133
    128 bank: 19, block: 1 : 0x0
    128 bank: 20, block: 0 : 0xc0002143
    128 bank: 20, block: 1 : 0x0
    128 bank: 21, block: 0 : 0xc0002153
    128 bank: 21, block: 1 : 0x0
    128 bank: 22, block: 0 : 0xc0002163
    128 bank: 22, block: 1 : 0x0

so you have 128 times the same address on a 128 core Zen box.

If that is correct, then we can use this nice simplification and cache them globally and not per-CPU. Seems to work here. Scream if something's amiss.

Thx.
---
From d8614e904d8f63b89d2d204d6fa247c4c416a1b6 Mon Sep 17 00:00:00 2001
From: Borislav Petkov <bp@susede>
Date: Thu, 17 May 2018 10:46:26 +0200
Subject: [PATCH] array caching

Not-Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 29 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 15 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +14 ~~-15~~

index f7666eef4a87..c8e038800591 100644
@@ -94,6 +94,11 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_SMU]	= { "smu",		"System Management Unit" },
 };
 
+static u32 smca_bank_addrs[MAX_NR_BANKS][NR_BLOCKS] __ro_after_init =
+{
+	[0 ... MAX_NR_BANKS - 1] = { [0 ... NR_BLOCKS - 1] = -1 }
+};
+
 const char *smca_get_name(enum smca_bank_types t)
 {
 	if (t >= N_SMCA_BANK_TYPES)

@@ -443,20 +448,26 @@ static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
 	if (!block)
 		return MSR_AMD64_SMCA_MCx_MISC(bank);
 
+	/* Check our cache first: */
+	if (smca_bank_addrs[bank][block] != -1)
+		return smca_bank_addrs[bank][block];
+
 	/*
 	 * For SMCA enabled processors, BLKPTR field of the first MISC register
 	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-4).
 	 */
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
-		return addr;
+		goto out;
 
 	if (!(low & MCI_CONFIG_MCAX))
-		return addr;
+		goto out;
 
 	if (!rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high) &&
 	    (low & MASK_BLKPTR_LO))
-		return MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
+		addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
 
+out:
+	smca_bank_addrs[bank][block] = addr;
 	return addr;
 }

@@ -468,18 +479,6 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
 		return addr;
 
-	/* Get address from already initialized block. */
-	if (per_cpu(threshold_banks, cpu)) {
-		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
-
-		if (bankp && bankp->blocks) {
-			struct threshold_block *blockp = &bankp->blocks[block];
-
-			if (blockp)
-				return blockp->address;
-		}
-	}
-
 	if (mce_flags.smca)
 		return smca_get_block_address(cpu, bank, block);

Ghannam, Yazen

author maintainer

2018-05-17 13:04 UTC #13 lore raw

RE: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 10 lines

-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Thursday, May 17, 2018 6:41 AM
To: Johannes Hirte <[email protected]>
Cc: Ghannam, Yazen <[email protected]>; linux- [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 8 lines

Quoted 4 lines · Johannes Hirte
Maybe I'm missing something, but those RDMSR IPSs don't happen on pre-SMCA systems, right? So the caching should be avoided here, cause the whole lookup looks more expensive to me than the simple switch-block in get_block_address.
Yeah, and we should simply cache all those MSR values as I suggested then.

Yes, you're right. I thought using the existing data structures would work, but it seems I messed up the implementation.

Quoted 30 lines

The patch at the end should fix your issue.

Yazen, so I'm working on the assumption that all addresses are the same on every core - I dumped them and it looks like this:

    128 bank: 00, block: 0 : 0xc0002003
    128 bank: 00, block: 1 : 0x0
    128 bank: 01, block: 0 : 0xc0002013
    128 bank: 01, block: 1 : 0x0
    128 bank: 02, block: 0 : 0xc0002023
    128 bank: 02, block: 1 : 0x0
    128 bank: 03, block: 0 : 0xc0002033
    128 bank: 03, block: 1 : 0x0
    128 bank: 04, block: 0 : 0x0
    128 bank: 05, block: 0 : 0xc0002053
    128 bank: 05, block: 1 : 0x0
    128 bank: 06, block: 0 : 0xc0002063
    128 bank: 06, block: 1 : 0x0
    128 bank: 07, block: 0 : 0xc0002073
    128 bank: 07, block: 1 : 0x0
    128 bank: 08, block: 0 : 0xc0002083
    128 bank: 08, block: 1 : 0x0
    128 bank: 09, block: 0 : 0xc0002093
    128 bank: 09, block: 1 : 0x0
    128 bank: 10, block: 0 : 0xc00020a3
    128 bank: 10, block: 1 : 0x0
    128 bank: 11, block: 0 : 0xc00020b3
    128 bank: 11, block: 1 : 0x0
    128 bank: 12, block: 0 : 0xc00020c3
    128 bank: 12, block: 1 : 0x0

Quoted 8 lines

    128 bank: 13, block: 0 : 0xc00020d3
    128 bank: 13, block: 1 : 0x0
    128 bank: 14, block: 0 : 0xc00020e3
    128 bank: 14, block: 1 : 0x0
    128 bank: 15, block: 0 : 0xc00020f3
    128 bank: 15, block: 1 : 0x0
    128 bank: 16, block: 0 : 0xc0002103
    128 bank: 16, block: 1 : 0x0

Banks 15 and 16 should have an address for block 1 also. Do you have PFEH enabled on your system? That would cause MISC0 to be RAZ so we won't get the MISC1 address. I'll try it myself also and let you know.

Quoted 13 lines

    128 bank: 17, block: 0 : 0xc0002113
    128 bank: 17, block: 1 : 0x0
    128 bank: 18, block: 0 : 0xc0002123
    128 bank: 18, block: 1 : 0x0
    128 bank: 19, block: 0 : 0xc0002133
    128 bank: 19, block: 1 : 0x0
    128 bank: 20, block: 0 : 0xc0002143
    128 bank: 20, block: 1 : 0x0
    128 bank: 21, block: 0 : 0xc0002153
    128 bank: 21, block: 1 : 0x0
    128 bank: 22, block: 0 : 0xc0002163
    128 bank: 22, block: 1 : 0x0

Quoted 6 lines

so you have 128 times the same address on a 128 core Zen box.
If that is correct, then we can use this nice simplification and cache them globally and not per-CPU. Seems to work here. Scream if something's amiss.

I think this good for now. We'll probably need to change it in the future, but maybe we can clean up all the thresholding blocks code and make it simpler when we do change it.

Quoted 32 lines

Thx.
---
From d8614e904d8f63b89d2d204d6fa247c4c416a1b6 Mon Sep 17 00:00:00 2001
From: Borislav Petkov <bp@susede>
Date: Thu, 17 May 2018 10:46:26 +0200
Subject: [PATCH] array caching
Not-Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 29 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 15 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +5 -0
index f7666eef4a87..c8e038800591 100644
@@ -94,6 +94,11 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_SMU]	= { "smu",		"System Management Unit" },
 };
 
+static u32 smca_bank_addrs[MAX_NR_BANKS][NR_BLOCKS] __ro_after_init =
+{
+	[0 ... MAX_NR_BANKS - 1] = { [0 ... NR_BLOCKS - 1] = -1 }
+};
+
 const char *smca_get_name(enum smca_bank_types t)
 {
 	if (t >= N_SMCA_BANK_TYPES)

arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 smca_get_block_address(unsigned int · line 455

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +4 -0

@@ -443,20 +448,26 @@ static u32 smca_get_block_address(unsigned int
 cpu, unsigned int bank,
 	if (!block)
 		return MSR_AMD64_SMCA_MCx_MISC(bank);
 
+	/* Check our cache first: */
+	if (smca_bank_addrs[bank][block] != -1)
+		return smca_bank_addrs[bank][block];
+

This hunk could go above the !block. Though maybe the macro is lighter than the array lookup. It'll work either way, but I'm just thinking out loud.

Quoted 17 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0

 	/*
 	 * For SMCA enabled processors, BLKPTR field of the first MISC register
 	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-

4).

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0

 	 */
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_CONFIG(bank),

&low, &high))

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -2

-		return addr;
+		goto out;
 
 	if (!(low & MCI_CONFIG_MCAX))
-		return addr;
+		goto out;
 
 	if (!rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_MISC(bank),

&low, &high) &&

arch/x86/kernel/cpu/mcheck/mce_amd.c static u32 smca_get_block_address(unsigned int cpu, unsigned int bank, · line 472

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +3 -1

 	    (low & MASK_BLKPTR_LO))
-		return MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
+		addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
 
+out:
+	smca_bank_addrs[bank][block] = addr;
 	return addr;
 }

Since we're caching the values during init, we can drop all the *_on_cpu() calls. What do you think?

Thanks,
Yazen

Borislav Petkov

2018-05-17 13:44 UTC #14 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 2 lines · Yazen

Yes, you're right. I thought using the existing data structures would work, but it seems I messed up the implementation.

Not only that - your idea wouldn't fly because the per-CPU stuff you were using gets torn down when the CPU goes offline so you can't use them on resume because they're not there yet.

Quoted 3 lines

Banks 15 and 16 should have an address for block 1 also. Do you have PFEH enabled on your system? That would cause MISC0 to be RAZ so we won't get the MISC1 address. I'll try it myself also and let you know.

I check PFEH is enabled how?

Quoted 3 lines

I think this good for now. We'll probably need to change it in the future, but maybe we can clean up all the thresholding blocks code and make it simpler when we do change it.

Ok.

Quoted 2 lines

This hunk could go above the !block. Though maybe the macro is lighter than the array lookup. It'll work either way, but I'm just thinking out loud.

Yeah, it doesn't matter in that case.

Quoted 2 lines

Since we're caching the values during init, we can drop all the *_on_cpu() calls. What do you think?

Well, if they're all the same on all CPUs, sure. That's your call.

Ghannam, Yazen

author maintainer

2018-05-17 14:05 UTC #15 lore raw

RE: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 11 lines

-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Thursday, May 17, 2018 9:44 AM
To: Ghannam, Yazen <[email protected]>
Cc: Johannes Hirte <[email protected]>; linux- [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block
On Thu, May 17, 2018 at 01:04:19PM +0000, Ghannam, Yazen wrote:

...

Quoted 3 lines

I check PFEH is enabled how?

If MISC0 is RAZ then you can assume PFEH is enabled. There should be a BIOS option to disable it.

BTW, I just tried you patch with PFEH disabled and it seems to work fine.

...

Quoted 5 lines

Quoted 2 lines
Since we're caching the values during init, we can drop all the *_on_cpu() calls. What do you think?
Well, if they're all the same on all CPUs, sure. That's your call.

Let's drop them. We won't need them since we're caching the values during init. And the init code is run on the target CPU.

We can just make smca_bank_addrs[][] into per_cpu when we need to support different values on different CPUs.

Thanks,
Yazen

Borislav Petkov

2018-05-17 18:30 UTC #16 lore raw

PATCH 1/2 x86/MCE/AMD: Cache SMCA MISC block addresses

From: Borislav Petkov <[email protected]>

... into a global, two-dimensional array and service subsequent reads from that cache to avoid rdmsr_on_cpu() calls during CPU hotplug (IPIs with IRQs disabled).

In addition, this fixes a KASAN slab-out-of-bounds read due to wrong usage of the bank->blocks pointer.

Reported-by Johannes Hirte <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Cc Yazen Ghannam <[email protected]>

Fixes 27bd59502702 ("x86/mce/AMD: Get address from already initialized block")

Link http://lkml.kernel.org/r/20180414004230.GA2033@probook open here

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 29 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 15 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +14 ~~-15~~

index f7666eef4a87..c8e038800591 100644
@@ -94,6 +94,11 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_SMU]	= { "smu",		"System Management Unit" },
 };
 
+static u32 smca_bank_addrs[MAX_NR_BANKS][NR_BLOCKS] __ro_after_init =
+{
+	[0 ... MAX_NR_BANKS - 1] = { [0 ... NR_BLOCKS - 1] = -1 }
+};
+
 const char *smca_get_name(enum smca_bank_types t)
 {
 	if (t >= N_SMCA_BANK_TYPES)

@@ -443,20 +448,26 @@ static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
 	if (!block)
 		return MSR_AMD64_SMCA_MCx_MISC(bank);
 
+	/* Check our cache first: */
+	if (smca_bank_addrs[bank][block] != -1)
+		return smca_bank_addrs[bank][block];
+
 	/*
 	 * For SMCA enabled processors, BLKPTR field of the first MISC register
 	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-4).
 	 */
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
-		return addr;
+		goto out;
 
 	if (!(low & MCI_CONFIG_MCAX))
-		return addr;
+		goto out;
 
 	if (!rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high) &&
 	    (low & MASK_BLKPTR_LO))
-		return MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
+		addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
 
+out:
+	smca_bank_addrs[bank][block] = addr;
 	return addr;
 }

@@ -468,18 +479,6 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
 		return addr;
 
-	/* Get address from already initialized block. */
-	if (per_cpu(threshold_banks, cpu)) {
-		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
-
-		if (bankp && bankp->blocks) {
-			struct threshold_block *blockp = &bankp->blocks[block];
-
-			if (blockp)
-				return blockp->address;
-		}
-	}
-
 	if (mce_flags.smca)
 		return smca_get_block_address(cpu, bank, block);

Borislav Petkov

2018-05-17 18:31 UTC #17 lore raw

PATCH 2/2 x86/MCE/AMD: Read MCx_MISC block addresses on any CPU

In reply to Ghannam, Yazen

From: Borislav Petkov <[email protected]>

We used rdmsr_safe_on_cpu() to make sure we're reading the proper CPU's MISC block addresses. However, that caused trouble with CPU hotplug due to the _on_cpu() helper issuing an IPI while IRQs are disabled.

But we don't have to do that: the block addresses are the same on any CPU so we can read them on any CPU. (What practically happens is, we read them on the BSP and cache them, and for later reads, we service them from the cache).

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +7 -8

index c8e038800591..f591b01930db 100644
@@ -436,8 +436,7 @@ static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c)
 	wrmsr(MSR_CU_DEF_ERR, low, high);
 }
 
-static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
-				  unsigned int block)
+static u32 smca_get_block_address(unsigned int bank, unsigned int block)
 {
 	u32 low, high;
 	u32 addr = 0;

@@ -456,13 +455,13 @@ static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
 	 * For SMCA enabled processors, BLKPTR field of the first MISC register
 	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-4).
 	 */
-	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
+	if (rdmsr_safe(MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
 		goto out;
 
 	if (!(low & MCI_CONFIG_MCAX))
 		goto out;
 
-	if (!rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high) &&
+	if (!rdmsr_safe(MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high) &&
 	    (low & MASK_BLKPTR_LO))
 		addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);

@@ -471,7 +470,7 @@ static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
 	return addr;
 }
 
-static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 high,
+static u32 get_block_address(u32 current_addr, u32 low, u32 high,
 			     unsigned int bank, unsigned int block)
 {
 	u32 addr = 0, offset = 0;

@@ -480,7 +479,7 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 		return addr;
 
 	if (mce_flags.smca)
-		return smca_get_block_address(cpu, bank, block);
+		return smca_get_block_address(bank, block);
 
 	/* Fall back to method we used for older processors: */
 	switch (block) {

@@ -558,7 +557,7 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 			smca_configure(bank, cpu);
 
 		for (block = 0; block < NR_BLOCKS; ++block) {
-			address = get_block_address(cpu, address, low, high, bank, block);
+			address = get_block_address(address, low, high, bank, block);
 			if (!address)
 				break;

@@ -1175,7 +1174,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	if (err)
 		goto out_free;
 recurse:
-	address = get_block_address(cpu, address, low, high, bank, ++block);
+	address = get_block_address(address, low, high, bank, ++block);
 	if (!address)
 		return 0;

Johannes Hirte

2018-05-17 19:29 UTC #18 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

In reply to Borislav Petkov

Quoted 10 lines · Borislav Petkov

Quoted 4 lines · Johannes Hirte
Maybe I'm missing something, but those RDMSR IPSs don't happen on pre-SMCA systems, right? So the caching should be avoided here, cause the whole lookup looks more expensive to me than the simple switch-block in get_block_address.
Yeah, and we should simply cache all those MSR values as I suggested then.
The patch at the end should fix your issue.

Works as expected on my Carrizo.

Borislav Petkov

2018-05-17 19:33 UTC #19 lore raw

Re: [PATCH 3/3] x86/MCE/AMD: Get address from already initialized block

Quoted 1 line · Johannes Hirte

Works as expected on my Carrizo.

Thanks, I'll add your Tested-by.

tip-bot for Borislav Petkov

merge bot

2018-05-19 13:22 UTC #20 lore raw

[tip:ras/urgent] x86/MCE/AMD: Cache SMCA MISC block addresses

In reply to Johannes Hirte

x86/MCE/AMD: Cache SMCA MISC block addresses

Commit-ID:  78ce241099bb363b19dbd0245442e66c8de8f567
Gitweb:     https://git.kernel.org/tip/78ce241099bb363b19dbd0245442e66c8de8f567
Author:     Borislav Petkov <[email protected]>
AuthorDate: Thu, 17 May 2018 10:46:26 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 19 May 2018 15:19:30 +0200

x86/MCE/AMD: Cache SMCA MISC block addresses

... into a global, two-dimensional array and service subsequent reads from that cache to avoid rdmsr_on_cpu() calls during CPU hotplug (IPIs with IRQs disabled).

In addition, this fixes a KASAN slab-out-of-bounds read due to wrong usage of the bank->blocks pointer.

Fixes 27bd59502702 ("x86/mce/AMD: Get address from already initialized block")

Reported-by Johannes Hirte <[email protected]>

Tested-by Johannes Hirte <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc Yazen Ghannam <[email protected]>

Link http://lkml.kernel.org/r/20180414004230.GA2033@probook open here

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +14 ~~-15~~

index f7666eef4a87..c8e038800591 100644
@@ -94,6 +94,11 @@ static struct smca_bank_name smca_names[] = {
 	[SMCA_SMU]	= { "smu",		"System Management Unit" },
 };
 
+static u32 smca_bank_addrs[MAX_NR_BANKS][NR_BLOCKS] __ro_after_init =
+{
+	[0 ... MAX_NR_BANKS - 1] = { [0 ... NR_BLOCKS - 1] = -1 }
+};
+
 const char *smca_get_name(enum smca_bank_types t)
 {
 	if (t >= N_SMCA_BANK_TYPES)

@@ -443,20 +448,26 @@ static u32 smca_get_block_address(unsigned int cpu, unsigned int bank,
 	if (!block)
 		return MSR_AMD64_SMCA_MCx_MISC(bank);
 
+	/* Check our cache first: */
+	if (smca_bank_addrs[bank][block] != -1)
+		return smca_bank_addrs[bank][block];
+
 	/*
 	 * For SMCA enabled processors, BLKPTR field of the first MISC register
 	 * (MCx_MISC0) indicates presence of additional MISC regs set (MISC1-4).
 	 */
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_CONFIG(bank), &low, &high))
-		return addr;
+		goto out;
 
 	if (!(low & MCI_CONFIG_MCAX))
-		return addr;
+		goto out;
 
 	if (!rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_MISC(bank), &low, &high) &&
 	    (low & MASK_BLKPTR_LO))
-		return MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
+		addr = MSR_AMD64_SMCA_MCx_MISCy(bank, block - 1);
 
+out:
+	smca_bank_addrs[bank][block] = addr;
 	return addr;
 }

@@ -468,18 +479,6 @@ static u32 get_block_address(unsigned int cpu, u32 current_addr, u32 low, u32 hi
 	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
 		return addr;
 
-	/* Get address from already initialized block. */
-	if (per_cpu(threshold_banks, cpu)) {
-		struct threshold_bank *bankp = per_cpu(threshold_banks, cpu)[bank];
-
-		if (bankp && bankp->blocks) {
-			struct threshold_block *blockp = &bankp->blocks[block];
-
-			if (blockp)
-				return blockp->address;
-		}
-	}
-
 	if (mce_flags.smca)
 		return smca_get_block_address(cpu, bank, block);

Borislav Petkov

maintainer

2018-02-08 15:04 UTC #21 lore raw

In reply to Yazen Ghannam

Quoted 8 lines · Yazen Ghannam

 arch/x86/kernel/cpu/mcheck/mce_amd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0

index 0f32ad242324..4e16afc0794d 100644

arch/x86/kernel/cpu/mcheck/mce_amd.c const char *smca_get_long_name(enum smca_bank_types t) · line 117

Quoted 10 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -2

@@ -110,14 +110,14 @@ const char *smca_get_long_name(enum smca_bank_types t)
 }
 EXPORT_SYMBOL_GPL(smca_get_long_name);
 
-static enum smca_bank_types smca_get_bank_type(struct mce *m)
+static enum smca_bank_types smca_get_bank_type(unsigned int bank)
 {
 	struct smca_bank *b;
 
-	if (m->bank >= N_SMCA_BANK_TYPES)
+	if (bank >= ARRAY_SIZE(smca_banks))

MAX_NR_BANKS

Ghannam, Yazen

author maintainer

2018-02-14 16:38 UTC #22 lore raw

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBCb3Jpc2xhdiBQZXRrb3YgW21h aWx0bzpicEBhbGllbjguZGVdDQo+IFNlbnQ6IFRodXJzZGF5LCBGZWJydWFyeSA4LCAyMDE4IDEw OjA1IEFNDQo+IFRvOiBHaGFubmFtLCBZYXplbiA8WWF6ZW4uR2hhbm5hbUBhbWQuY29tPg0KPiBD YzogbGludXgtZWRhY0B2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5v cmc7IGJwQHN1c2UuZGU7DQo+IHRvbnkubHVja0BpbnRlbC5jb207IHg4NkBrZXJuZWwub3JnDQo+ IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMS8zXSB4ODYvTUNFL0FNRDogUmVkbyBmdW5jdGlvbiB0byBn ZXQgU01DQSBiYW5rIHR5cGUNCj4gDQo+IE9uIFRodSwgRmViIDAxLCAyMDE4IGF0IDEyOjQ4OjEx UE0gLTA2MDAsIFlhemVuIEdoYW5uYW0gd3JvdGU6DQo+ID4gIGFyY2gveDg2L2tlcm5lbC9jcHUv bWNoZWNrL21jZV9hbWQuYyB8IDE1ICsrKysrKystLS0tLS0tLQ0KPiA+ICAxIGZpbGUgY2hhbmdl ZCwgNyBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBh L2FyY2gveDg2L2tlcm5lbC9jcHUvbWNoZWNrL21jZV9hbWQuYw0KPiBiL2FyY2gveDg2L2tlcm5l bC9jcHUvbWNoZWNrL21jZV9hbWQuYw0KPiA+IGluZGV4IDBmMzJhZDI0MjMyNC4uNGUxNmFmYzA3 OTRkIDEwMDY0NA0KPiA+IC0tLSBhL2FyY2gveDg2L2tlcm5lbC9jcHUvbWNoZWNrL21jZV9hbWQu Yw0KPiA+ICsrKyBiL2FyY2gveDg2L2tlcm5lbC9jcHUvbWNoZWNrL21jZV9hbWQuYw0KPiA+IEBA IC0xMTAsMTQgKzExMCwxNCBAQCBjb25zdCBjaGFyICpzbWNhX2dldF9sb25nX25hbWUoZW51bQ0K PiBzbWNhX2JhbmtfdHlwZXMgdCkNCj4gPiAgfQ0KPiA+ICBFWFBPUlRfU1lNQk9MX0dQTChzbWNh X2dldF9sb25nX25hbWUpOw0KPiA+DQo+ID4gLXN0YXRpYyBlbnVtIHNtY2FfYmFua190eXBlcyBz bWNhX2dldF9iYW5rX3R5cGUoc3RydWN0IG1jZSAqbSkNCj4gPiArc3RhdGljIGVudW0gc21jYV9i YW5rX3R5cGVzIHNtY2FfZ2V0X2JhbmtfdHlwZSh1bnNpZ25lZCBpbnQgYmFuaykNCj4gPiAgew0K PiA+ICAJc3RydWN0IHNtY2FfYmFuayAqYjsNCj4gPg0KPiA+IC0JaWYgKG0tPmJhbmsgPj0gTl9T TUNBX0JBTktfVFlQRVMpDQo+ID4gKwlpZiAoYmFuayA+PSBBUlJBWV9TSVpFKHNtY2FfYmFua3Mp KQ0KPiANCj4gTUFYX05SX0JBTktTDQo+IA0KDQpJIGtub3cgdGhhdCB3ZSdyZSBkZWNsYXJpbmcg c21jYV9iYW5rc1tdIHRvIGhhdmUgTUFYX05SX0JBTktTIGl0ZW1zLiBCdXQNCnNob3VsZG4ndCB3 ZSBkaXJlY3RseSBjaGVjayB0aGF0IGFuIGluZGV4IGlzIHdpdGhpbiB0aGUgYm91bmRzIG9mIHRo ZSBhcnJheT8NCldlJ2xsIGhhdmUgYSBidWcgaWYgd2UgY2hlY2sgYWdhaW5zdCBNQVhfTlJfQkFO S1MgYW5kIHRoZSBkZWZpbml0aW9uIG9mDQpzbWNhX2JhbmtzW10gY2hhbmdlcy4NCg0KVGhhbmtz LA0KWWF6ZW4NCg0K
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Borislav Petkov

2018-02-14 19:35 UTC #23 lore raw

Quoted 4 lines · Yazen

I know that we're declaring smca_banks[] to have MAX_NR_BANKS items. But shouldn't we directly check that an index is within the bounds of the array? We'll have a bug if we check against MAX_NR_BANKS and the definition of smca_banks[] changes.

Then we don't need MAX_NR_BANKS. We either keep them in sync or kill it.