[PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

If threshold_init_device() fails then per_cpu(threshold_banks) will be
deallocated. The thresholding interrupt handler will still be active, so
it's possible to get a NULL pointer dereference if a THR interrupt
happens and any of the structures are NULL.

Exit the handler if per_cpu(threshold_banks) is NULL and skip NULL
banks. MCA error information will still be in the registers. The
information will be logged during polling or in another MCA exception or
interrupt handler.

Fixes: 17ef4af0ec0f ("x86/mce/AMD: Use saved threshold block info in interrupt handler")
Cc: <stable@vger.kernel.org> # 4.13.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index dd33c357548f..2dbf34250bbf 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -934,13 +934,21 @@ static void log_and_reset_block(struct threshold_block *block)
 static void amd_threshold_interrupt(void)
 {
 	struct threshold_block *first_block = NULL, *block = NULL, *tmp = NULL;
+	struct threshold_bank *th_bank = NULL;
 	unsigned int bank, cpu = smp_processor_id();
 
+	if (!per_cpu(threshold_banks, cpu))
+		return;
+
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
 		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
 			continue;
 
-		first_block = per_cpu(threshold_banks, cpu)[bank]->blocks;
+		th_bank = per_cpu(threshold_banks, cpu)[bank];
+		if (!th_bank)
+			continue;
+
+		first_block = th_bank->blocks;
 		if (!first_block)
 			continue;
 
-- 
2.17.1

[PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

During mce_threshold_create_device() data structures are allocated for
each CPUs MCA banks and thresholding blocks. These data structures are
used to save information related to AMD's MCA Error Thresholding
feature. The structures are used in the thresholding interrupt handler,
and they are exposed to the user through sysfs. The sysfs interface has
user-friendly names for each bank.

However, errors in mce_threshold_create_device() will cause all the data
structures to be deallocated. This will break the thresholding interrupt
handler since it depends on these structures.

One possible error is creating a kobject with a NULL name. This will
happen if a bank exists on a system that doesn't have a name, e.g. new
bank types on future systems.

Skip creating kobjects for banks without a name. This means that the
sysfs interface for this bank will not exist. But this will keep all the
data structures allocated, so the thresholding interrupt handler will
work, even for the unnamed bank. Also, the sysfs interface will still be
populated for all existing, known bank types.

Cc: <stable@vger.kernel.org> # 4.13.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 2dbf34250bbf..521fd8f406df 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1130,6 +1130,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	struct threshold_block *b = NULL;
 	u32 low, high;
 	int err;
+	const char *name = NULL;
 
 	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
 		return 0;
@@ -1176,9 +1177,13 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
 	}
 
+	name = get_name(bank, b);
+	if (!name)
+		goto recurse;
+
 	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
 				   per_cpu(threshold_banks, cpu)[bank]->kobj,
-				   get_name(bank, b));
+				   name);
 	if (err)
 		goto out_free;
 recurse:
@@ -1265,12 +1270,16 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		goto out;
 	}
 
+	if (!name)
+		goto allocate;
+
 	b->kobj = kobject_create_and_add(name, &dev->kobj);
 	if (!b->kobj) {
 		err = -EINVAL;
 		goto out_free;
 	}
 
+allocate:
 	per_cpu(threshold_banks, cpu)[bank] = b;
 
 	if (is_shared_bank(bank)) {
-- 
2.17.1

Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 09, 2018 at 09:08:34AM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> During mce_threshold_create_device() data structures are allocated for
> each CPUs MCA banks and thresholding blocks. These data structures are
> used to save information related to AMD's MCA Error Thresholding
> feature. The structures are used in the thresholding interrupt handler,
> and they are exposed to the user through sysfs. The sysfs interface has
> user-friendly names for each bank.
> 
> However, errors in mce_threshold_create_device() will cause all the data
> structures to be deallocated. This will break the thresholding interrupt
> handler since it depends on these structures.

Same argument as before: if our init fails in some fashion, we should
not be running the interrupt handler.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org>
> On Behalf Of Borislav Petkov
> Sent: Thursday, August 9, 2018 11:18 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 09, 2018 at 09:08:34AM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > During mce_threshold_create_device() data structures are allocated for
> > each CPUs MCA banks and thresholding blocks. These data structures are
> > used to save information related to AMD's MCA Error Thresholding
> > feature. The structures are used in the thresholding interrupt handler,
> > and they are exposed to the user through sysfs. The sysfs interface has
> > user-friendly names for each bank.
> >
> > However, errors in mce_threshold_create_device() will cause all the data
> > structures to be deallocated. This will break the thresholding interrupt
> > handler since it depends on these structures.
> 
> Same argument as before: if our init fails in some fashion, we should
> not be running the interrupt handler.
> 

This patch makes it so that we don't fail init just because some banks don't
have names. The data caching we do is useful even if we fail to create sysfs
entries for some banks. The interrupt handler can work fine without a sysfs
entry for every bank. It seems like overkill to deallocate all the structures
and sysfs entries for all the banks just because we fail to create entries for
some banks that don't have names. 

In other words, I think we should decouple the interrupt handler from the
sysfs interface. The interface is nice to have but not necessary for the HW
and OS to handle threshold interrupts. If we do so, then new HW with new,
unnamed types will work with older versions of Linux. We can then add the
new type names without having to backport to fix the interrupt handler.

Thanks,
Yazen

Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 09, 2018 at 06:46:26PM +0000, Ghannam, Yazen wrote:
> This patch makes it so that we don't fail init just because some banks don't
> have names. The data caching we do is useful even if we fail to create sysfs
> entries for some banks. The interrupt handler can work fine without a sysfs
> entry for every bank. It seems like overkill to deallocate all the structures
> and sysfs entries for all the banks just because we fail to create entries for
> some banks that don't have names.

Maybe I'm missing the big picture here but why, all of a sudden, are
some banks without names? This clearly is new because it wasn't like
that before, so what is it? Maybe you should explain the bigger picture
first.

And if banks don't have names, then we should generate some for them
instead. Because this code is already ugly and recursive - we certainly
don't want to add more conditionals to it because that thing is a mess
as it is now.

> In other words, I think we should decouple the interrupt handler from the
> sysfs interface. The interface is nice to have but not necessary for the HW
> and OS to handle threshold interrupts. If we do so, then new HW with new,
> unnamed types will work with older versions of Linux.

To tell you the truth, I'm not at all psyched about telling the future.
We can try to be future-proof to a certain degree but this should not be
the determining factor how we design things.

But the aspect of decoupling sysfs representation from handler makes
sense. It just needs to be done cleanly.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org>
> On Behalf Of Borislav Petkov
> Sent: Wednesday, August 15, 2018 10:55 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 09, 2018 at 06:46:26PM +0000, Ghannam, Yazen wrote:
> > This patch makes it so that we don't fail init just because some banks don't
> > have names. The data caching we do is useful even if we fail to create sysfs
> > entries for some banks. The interrupt handler can work fine without a sysfs
> > entry for every bank. It seems like overkill to deallocate all the structures
> > and sysfs entries for all the banks just because we fail to create entries for
> > some banks that don't have names.
> 
> Maybe I'm missing the big picture here but why, all of a sudden, are
> some banks without names? This clearly is new because it wasn't like
> that before, so what is it? Maybe you should explain the bigger picture
> first.
> 

Yes, you're right. I'll update the commit message.

Before Fam17h, we went generations with only a few MCA banks and their
bank numbers and types were consistent. With Fam17h and SMCA we got a
whole new set of bank types and now these banks may not have the same
ordering between models. So we created names for the new banks and we
match the banks to a name based on values from the MCA_IPID register.

Future SMCA systems may have banks whose MCA_IPID is not known so we
don't match a name to them. This could happen if we have a minor revision
to a known bank, or if we have a completely new bank type.

> And if banks don't have names, then we should generate some for them
> instead. Because this code is already ugly and recursive - we certainly
> don't want to add more conditionals to it because that thing is a mess
> as it is now.
> 

This is a good idea.

In the past, the sysfs entries were named something like "th_bank#" before
each bank was given a more descriptive name. We can go back to this format
as a fallback for unrecognized bank types. This will fix the issue with sysfs
failing and also the interface will still work on new hardware.

I can do this and update the commit message in a v2 patch.

> > In other words, I think we should decouple the interrupt handler from the
> > sysfs interface. The interface is nice to have but not necessary for the HW
> > and OS to handle threshold interrupts. If we do so, then new HW with new,
> > unnamed types will work with older versions of Linux.
> 
> To tell you the truth, I'm not at all psyched about telling the future.
> We can try to be future-proof to a certain degree but this should not be
> the determining factor how we design things.
> 
> But the aspect of decoupling sysfs representation from handler makes
> sense. It just needs to be done cleanly.
> 

I agree. I just wanted to make a small change and fix this issue. I think there's
a lot we can do to clean up the thresholding code going forward.

Thanks,
Yazen

Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 16, 2018 at 06:46:33PM +0000, Ghannam, Yazen wrote:
> Future SMCA systems may have banks whose MCA_IPID is not known so we

You lost me here. How would the IPID not be known? Are you talking about
an old kernel running on new HW and thus the HardwareID in its MCA_IPID
to be unknown to the old kernel?

> In the past, the sysfs entries were named something like "th_bank#" before
> each bank was given a more descriptive name. We can go back to this format
> as a fallback for unrecognized bank types. This will fix the issue with sysfs
> failing and also the interface will still work on new hardware.

You could do: th_bank#<HardwareID> so that we can find what it is by
grepping the PPR.

> I agree. I just wanted to make a small change and fix this issue. I
> think there's a lot we can do to clean up the thresholding code going
> forward.

ACK.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, August 21, 2018 8:16 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 16, 2018 at 06:46:33PM +0000, Ghannam, Yazen wrote:
> > Future SMCA systems may have banks whose MCA_IPID is not known so we
> 
> You lost me here. How would the IPID not be known? Are you talking about
> an old kernel running on new HW and thus the HardwareID in its MCA_IPID
> to be unknown to the old kernel?
> 

Yes, that's right: old kernel on new HW.

> > In the past, the sysfs entries were named something like "th_bank#" before
> > each bank was given a more descriptive name. We can go back to this format
> > as a fallback for unrecognized bank types. This will fix the issue with sysfs
> > failing and also the interface will still work on new hardware.
> 
> You could do: th_bank#<HardwareID> so that we can find what it is by
> grepping the PPR.
> 

Good idea. Will do.

Thanks,
Yazen

Re: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[Borislav Petkov]

On Thu, Aug 09, 2018 at 09:08:33AM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> If threshold_init_device() fails then per_cpu(threshold_banks) will be
> deallocated. The thresholding interrupt handler will still be active, so

So fix the code so that *that* doesn't happen instead of adding checks
to the interrupt handler.

I.e.,

	if (err) {
		mce_threshold_vector = default_threshold_interrupt;
		return err;
	}

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Thursday, August 9, 2018 11:16 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR
> interrupt handler
> 
> On Thu, Aug 09, 2018 at 09:08:33AM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > If threshold_init_device() fails then per_cpu(threshold_banks) will be
> > deallocated. The thresholding interrupt handler will still be active, so
> 
> So fix the code so that *that* doesn't happen instead of adding checks
> to the interrupt handler.
> 
> I.e.,
> 
> 	if (err) {
> 		mce_threshold_vector = default_threshold_interrupt;
> 		return err;
> 	}
> 

Okay. I'll make that change.

Thanks,
Yazen

RE: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org>
> On Behalf Of Ghannam, Yazen
> Sent: Thursday, August 9, 2018 1:18 PM
> To: Borislav Petkov <bp@alien8.de>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: RE: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR
> interrupt handler
> 
> > -----Original Message-----
> > From: Borislav Petkov <bp@alien8.de>
> > Sent: Thursday, August 9, 2018 11:16 AM
> > To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> > Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> > tony.luck@intel.com; x86@kernel.org
> > Subject: Re: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR
> > interrupt handler
> >
> > On Thu, Aug 09, 2018 at 09:08:33AM -0500, Yazen Ghannam wrote:
> > > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > >
> > > If threshold_init_device() fails then per_cpu(threshold_banks) will be
> > > deallocated. The thresholding interrupt handler will still be active, so
> >
> > So fix the code so that *that* doesn't happen instead of adding checks
> > to the interrupt handler.
> >
> > I.e.,
> >
> > 	if (err) {
> > 		mce_threshold_vector = default_threshold_interrupt;
> > 		return err;
> > 	}
> >
> 
> Okay. I'll make that change.
> 

I don't think this is enough. We have a gap between when the interrupt
handler is set up during boot in __mcheck_cpu_init_vendor() and when all
the data structures are created during threshold_init_device().

So I think we should keep the NULL pointer checks for now to keep this fix
small. I can make a new patch following your suggestion above.

We can change the code so that we create the data structures during the
earlier init process, but I think this will be a much bigger change. This could
fall under the idea of decoupling the handling code from sysfs.

Thanks,
Yazen

Re: [PATCH 1/2] x86/MCE/AMD: Check for NULL banks in THR interrupt handler

[Borislav Petkov]

On Thu, Aug 16, 2018 at 07:00:43PM +0000, Ghannam, Yazen wrote:
> So I think we should keep the NULL pointer checks for now to keep this fix
> small. I can make a new patch following your suggestion above.
> 
> We can change the code so that we create the data structures during the
> earlier init process, but I think this will be a much bigger change. This could
> fall under the idea of decoupling the handling code from sysfs.

Ok, then pls add the simpler fixes at the beginning of the series so
that they can go to stable and have the more involved changes follow
them.

Thx.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--