x86, microcode: Conversion from sysdev class caused regression

x86, microcode: Conversion from sysdev class caused regression

[Andreas Herrmann]

Hi,

commit 8a25a2fd126c621f44f3aeaef80d51f00fc11639 (cpu: convert 'cpu'
and 'machinecheck' sysdev_class to a regular subsystem) causes a
regression in the CPU microcode loader.

Previous behaviour was that module loading failed if no appropriate
CPU was found. Now we get

 [  131.717212] WARNING: at fs/sysfs/group.c:138 mc_device_remove+0x5f/0x70 [microcode]()
 [  131.717218] Hardware name: 01972NG
 [  131.717223] sysfs group ffffffffa00013d0 not found for kobject 'cpu0'
 [  131.717228] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel
                btusb snd_hda_codec bluetooth thinkpad_acpi rfkill microcode(-) [last unloaded: cfg80211]
 [  131.717254] Pid: 4560, comm: modprobe Not tainted 3.4.0-rc2-00002-g258f742 #5
 [  131.717260] Call Trace:
 [  131.717277]  [<ffffffff8103113b>] ? warn_slowpath_common+0x7b/0xc0
 [  131.717287]  [<ffffffff81031235>] ? warn_slowpath_fmt+0x45/0x50
 [  131.717300]  [<ffffffff81120e74>] ? sysfs_remove_group+0x34/0x120
 [  131.717311]  [<ffffffffa00000ef>] ? mc_device_remove+0x5f/0x70 [microcode]
 [  131.717325]  [<ffffffff81331eb9>] ? subsys_interface_unregister+0x69/0xa0
 [  131.717336]  [<ffffffff81563526>] ? mutex_lock+0x16/0x40
 [  131.717346]  [<ffffffffa0000c3e>] ? microcode_exit+0x50/0x92 [microcode]
 [  131.717357]  [<ffffffff8107051d>] ? sys_delete_module+0x16d/0x260
 [  131.717369]  [<ffffffff810a0065>] ? wait_iff_congested+0x45/0x110
 [  131.717379]  [<ffffffff815656af>] ? page_fault+0x1f/0x30
 [  131.717389]  [<ffffffff81565ba2>] ? system_call_fastpath+0x16/0x1b


after loading the driver on an unsupported CPU and the subsequent
attempt to unload the driver.

The reason for the error is that subsys_interface_register() doesn't
handle the return value of sif->add_dev (and there's also no unwinding
of the interface registration). Instead subsys_interface_register
always returns 0. This is different to what we had for
sysdev_driver_register(). The latter properly handled return codes of
drv->add() functions.

See Boris' commit f4203e3032e5ae74c3e89df85a5a6d96022d0c49 (sysdev: Do
not register with sysdev when erroring on add).

I don't know whether the microcode driver is the only driver that
is affected.

What's your suggestion for a fix? Should subsys_interface_register()
be fixed or should each affected driver be adapted?


Regards,
Andreas

Re: x86, microcode: Conversion from sysdev class caused regression

[Kay Sievers]

On Wed, Apr 11, 2012 at 18:38, Andreas Herrmann
<andreas.herrmann3@amd.com> wrote:

> The reason for the error is that subsys_interface_register() doesn't
> handle the return value of sif->add_dev (and there's also no unwinding
> of the interface registration). Instead subsys_interface_register
> always returns 0.

Which is the intended behaviour of 'subsystem interfaces' from the
driver-core perspective. It should not matter if one of a bunch of
devices do not 'like' this 'interface'. It is the same model as a
'driver', we do not cancel the link-in of a driver if one device does
not like the driver.

> This is different to what we had for
> sysdev_driver_register(). The latter properly handled return codes of
> drv->add() functions.

A 'subsystem interface' is a global instance per subsystem, not
something _all_ devices must agree on, to be registered. We can load
almost all drivers in the kernel, without even having a device around;
the 'interfaces' should not be that different in their model.

> See Boris' commit f4203e3032e5ae74c3e89df85a5a6d96022d0c49 (sysdev: Do
> not register with sysdev when erroring on add).

It seems to paper over a bug of the microcode driver, we should not do
that, or make such assumptions.

> I don't know whether the microcode driver is the only driver that
> is affected.

All interfaces with the same assumption of the microcode driver might,
but see below.

> What's your suggestion for a fix? Should subsys_interface_register()
> be fixed or should each affected driver be adapted?

All the drivers, if there are any more, who are as naive as the
microcode driver. :)

"Subsystem interfaces' users must not assume that they can blindly
unwind their own stuff, which they have never registered.
mc_device_remove() just goes ahead without any check and calls
sysfs_remove_group(), while it should check its own state of this
specific CPU it acts on.

I think a quick return in the microcode driver, for a device which has
no active interface state is the best solution here.

Thanks,
Kay

Re: x86, microcode: Conversion from sysdev class caused regression

[Borislav Petkov]

+ Dave.

On Wed, Apr 11, 2012 at 07:06:21PM +0200, Kay Sievers wrote:
> > The reason for the error is that subsys_interface_register() doesn't
> > handle the return value of sif->add_dev (and there's also no unwinding
> > of the interface registration). Instead subsys_interface_register
> > always returns 0.
>
> Which is the intended behaviour of 'subsystem interfaces' from the
> driver-core perspective. It should not matter if one of a bunch of
> devices do not 'like' this 'interface'. It is the same model as a
> 'driver', we do not cancel the link-in of a driver if one device does
> not like the driver.

But you're not looking at the return value of sif->add_dev which looks
strange to me. Let me put it this way: why do you have return values to
->add_dev's interface then if you're not going to look at them?

A warning that one of the dev_add's failed could probably make sense
here...

[..]

> I think a quick return in the microcode driver, for a device which has
> no active interface state is the best solution here.

Actually, it is even easier: the code clumsily does:

sysfs_create_group
microcode_init_cpu
if (err)
	sysfs_remove_group

so we go and create sysfs group, THEN check whether this CPU is
supported and if not, remove the group again which is a bunch of crap if
you ask me. The right way to go should be:

microcode_init_cpu
if (err)
	return;
sysfs_create_group

and then there's no need to do all that sysfs group dancing. Andreas,
let me know if you wanna do it, or I should take care of it.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: x86, microcode: Conversion from sysdev class caused regression

[Greg Kroah-Hartman]

On Wed, Apr 11, 2012 at 06:38:50PM +0200, Andreas Herrmann wrote:
> Hi,
> 
> commit 8a25a2fd126c621f44f3aeaef80d51f00fc11639 (cpu: convert 'cpu'
> and 'machinecheck' sysdev_class to a regular subsystem) causes a
> regression in the CPU microcode loader.
> 
> Previous behaviour was that module loading failed if no appropriate
> CPU was found. Now we get
> 
>  [  131.717212] WARNING: at fs/sysfs/group.c:138 mc_device_remove+0x5f/0x70 [microcode]()
>  [  131.717218] Hardware name: 01972NG
>  [  131.717223] sysfs group ffffffffa00013d0 not found for kobject 'cpu0'
>  [  131.717228] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel
>                 btusb snd_hda_codec bluetooth thinkpad_acpi rfkill microcode(-) [last unloaded: cfg80211]
>  [  131.717254] Pid: 4560, comm: modprobe Not tainted 3.4.0-rc2-00002-g258f742 #5
>  [  131.717260] Call Trace:
>  [  131.717277]  [<ffffffff8103113b>] ? warn_slowpath_common+0x7b/0xc0
>  [  131.717287]  [<ffffffff81031235>] ? warn_slowpath_fmt+0x45/0x50
>  [  131.717300]  [<ffffffff81120e74>] ? sysfs_remove_group+0x34/0x120
>  [  131.717311]  [<ffffffffa00000ef>] ? mc_device_remove+0x5f/0x70 [microcode]
>  [  131.717325]  [<ffffffff81331eb9>] ? subsys_interface_unregister+0x69/0xa0
>  [  131.717336]  [<ffffffff81563526>] ? mutex_lock+0x16/0x40
>  [  131.717346]  [<ffffffffa0000c3e>] ? microcode_exit+0x50/0x92 [microcode]
>  [  131.717357]  [<ffffffff8107051d>] ? sys_delete_module+0x16d/0x260
>  [  131.717369]  [<ffffffff810a0065>] ? wait_iff_congested+0x45/0x110
>  [  131.717379]  [<ffffffff815656af>] ? page_fault+0x1f/0x30
>  [  131.717389]  [<ffffffff81565ba2>] ? system_call_fastpath+0x16/0x1b
> 
> 
> after loading the driver on an unsupported CPU and the subsequent
> attempt to unload the driver.

That's not nice, but why is the driver getting loaded for a CPU that it
doesn't control?  Startup scripts?

It's just a "warning", everything else still works properly, right?

Patches to fix this up are always gladly accepted :)

thanks,

greg k-h

Re: x86, microcode: Conversion from sysdev class caused regression

[Borislav Petkov]

On Wed, Apr 11, 2012 at 01:06:01PM -0700, Greg Kroah-Hartman wrote:
> That's not nice, but why is the driver getting loaded for a CPU that it
> doesn't control?  Startup scripts?
> 
> It's just a "warning", everything else still works properly, right?

Yes.

> Patches to fix this up are always gladly accepted :)

Yeah, we'll send you a fix probably tomorrow.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: x86, microcode: Conversion from sysdev class caused regression

[Borislav Petkov]

On Wed, Apr 11, 2012 at 10:10:54PM +0200, Borislav Petkov wrote:
> Yeah, we'll send you a fix probably tomorrow.

Ok, we ended up doing two patches, I'm sending them as a reply to this
note. Will send them to Ingo if there are no complaints.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[PATCH 1/2] x86, microcode: Fix sysfs warning during module unload

[Borislav Petkov]

From: Andreas Herrmann <andreas.herrmann3@amd.com>
Date: Thu, 12 Apr 2012 16:48:01 +0200
Subject: [PATCH 1/2] x86, microcode: Fix sysfs warning during module unload on unsupported CPUs

Loading microcode driver on an unsupported CPU and subsequently
unloading the driver causes

 WARNING: at fs/sysfs/group.c:138 mc_device_remove+0x5f/0x70 [microcode]()
 Hardware name: 01972NG
 sysfs group ffffffffa00013d0 not found for kobject 'cpu0'
 Modules linked in: snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel btusb snd_hda_codec bluetooth thinkpad_acpi rfkill microcode(-) [last unloaded: cfg80211]
 Pid: 4560, comm: modprobe Not tainted 3.4.0-rc2-00002-g258f742 #5
 Call Trace:
  [<ffffffff8103113b>] ? warn_slowpath_common+0x7b/0xc0
  [<ffffffff81031235>] ? warn_slowpath_fmt+0x45/0x50
  [<ffffffff81120e74>] ? sysfs_remove_group+0x34/0x120
  [<ffffffffa00000ef>] ? mc_device_remove+0x5f/0x70 [microcode]
  [<ffffffff81331eb9>] ? subsys_interface_unregister+0x69/0xa0
  [<ffffffff81563526>] ? mutex_lock+0x16/0x40
  [<ffffffffa0000c3e>] ? microcode_exit+0x50/0x92 [microcode]
  [<ffffffff8107051d>] ? sys_delete_module+0x16d/0x260
  [<ffffffff810a0065>] ? wait_iff_congested+0x45/0x110
  [<ffffffff815656af>] ? page_fault+0x1f/0x30
  [<ffffffff81565ba2>] ? system_call_fastpath+0x16/0x1b

on recent kernels.

This is due to

  commit 8a25a2fd126c621f44f3aeaef80d51f00fc11639
  (cpu: convert 'cpu' and 'machinecheck' sysdev_class to a regular
  subsystem)

which renders

 commit 6c53cbfced048c421e4f72cb2183465f68fbc5e7
 (x86, microcode: Correct sysdev_add error path)

useless.

See http://marc.info/?l=linux-kernel&m=133416246406478

Avoid above warning by restoring the old driver behaviour before
commit 6c53cbfced048c421e4f72cb2183465f68fbc5e7 (x86, microcode:
Correct sysdev_add error path)

Cc: stable@vger.kernel.org
Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/kernel/microcode_core.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 87a0f86..d389e74 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -419,10 +419,8 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
 	if (err)
 		return err;
 
-	if (microcode_init_cpu(cpu) == UCODE_ERROR) {
-		sysfs_remove_group(&dev->kobj, &mc_attr_group);
+	if (microcode_init_cpu(cpu) == UCODE_ERROR)
 		return -EINVAL;
-	}
 
 	return err;
 }
-- 
1.7.8.4

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Borislav Petkov]

From: Andreas Herrmann <andreas.herrmann3@amd.com>
Date: Thu, 12 Apr 2012 16:51:57 +0200
Subject: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for supported AMD CPUs

Exit early when there's no support for a particular CPU family. Also,
fixup the "no support for this CPU vendor" to be issued only when the
driver is attempted to be loaded on an unsupported vendor.

Cc: stable@vger.kernel.org
Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
Cc: Borislav Petkov <borislav.petkov@amd.com>
[Boris: added a commit msg because Andreas is lazy]
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/kernel/microcode_amd.c  |   12 +++++++-----
 arch/x86/kernel/microcode_core.c |    6 +++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 73465aa..8a2ce8f 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -82,11 +82,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
-	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
-		pr_warning("CPU%d: family %d not supported\n", cpu, c->x86);
-		return -1;
-	}
-
 	csig->rev = c->microcode;
 	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
 
@@ -380,6 +375,13 @@ static struct microcode_ops microcode_amd_ops = {
 
 struct microcode_ops * __init init_amd_microcode(void)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
+		pr_warning("AMD CPU family 0x%x not supported\n", c->x86);
+		return NULL;
+	}
+
 	patch = (void *)get_zeroed_page(GFP_KERNEL);
 	if (!patch)
 		return NULL;
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index d389e74..c9bda6d 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -526,11 +526,11 @@ static int __init microcode_init(void)
 		microcode_ops = init_intel_microcode();
 	else if (c->x86_vendor == X86_VENDOR_AMD)
 		microcode_ops = init_amd_microcode();
-
-	if (!microcode_ops) {
+	else
 		pr_err("no support for this CPU vendor\n");
+
+	if (!microcode_ops)
 		return -ENODEV;
-	}
 
 	microcode_pdev = platform_device_register_simple("microcode", -1,
 							 NULL, 0);
-- 
1.7.8.4

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 1/2] x86, microcode: Fix sysfs warning during module unload

[Greg Kroah-Hartman]

On Thu, Apr 12, 2012 at 06:30:38PM +0200, Borislav Petkov wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> Date: Thu, 12 Apr 2012 16:48:01 +0200
> Subject: [PATCH 1/2] x86, microcode: Fix sysfs warning during module unload on unsupported CPUs
> 
> Loading microcode driver on an unsupported CPU and subsequently
> unloading the driver causes
> 
>  WARNING: at fs/sysfs/group.c:138 mc_device_remove+0x5f/0x70 [microcode]()
>  Hardware name: 01972NG
>  sysfs group ffffffffa00013d0 not found for kobject 'cpu0'
>  Modules linked in: snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel btusb snd_hda_codec bluetooth thinkpad_acpi rfkill microcode(-) [last unloaded: cfg80211]
>  Pid: 4560, comm: modprobe Not tainted 3.4.0-rc2-00002-g258f742 #5
>  Call Trace:
>   [<ffffffff8103113b>] ? warn_slowpath_common+0x7b/0xc0
>   [<ffffffff81031235>] ? warn_slowpath_fmt+0x45/0x50
>   [<ffffffff81120e74>] ? sysfs_remove_group+0x34/0x120
>   [<ffffffffa00000ef>] ? mc_device_remove+0x5f/0x70 [microcode]
>   [<ffffffff81331eb9>] ? subsys_interface_unregister+0x69/0xa0
>   [<ffffffff81563526>] ? mutex_lock+0x16/0x40
>   [<ffffffffa0000c3e>] ? microcode_exit+0x50/0x92 [microcode]
>   [<ffffffff8107051d>] ? sys_delete_module+0x16d/0x260
>   [<ffffffff810a0065>] ? wait_iff_congested+0x45/0x110
>   [<ffffffff815656af>] ? page_fault+0x1f/0x30
>   [<ffffffff81565ba2>] ? system_call_fastpath+0x16/0x1b
> 
> on recent kernels.
> 
> This is due to
> 
>   commit 8a25a2fd126c621f44f3aeaef80d51f00fc11639
>   (cpu: convert 'cpu' and 'machinecheck' sysdev_class to a regular
>   subsystem)
> 
> which renders
> 
>  commit 6c53cbfced048c421e4f72cb2183465f68fbc5e7
>  (x86, microcode: Correct sysdev_add error path)
> 
> useless.
> 
> See http://marc.info/?l=linux-kernel&m=133416246406478
> 
> Avoid above warning by restoring the old driver behaviour before
> commit 6c53cbfced048c421e4f72cb2183465f68fbc5e7 (x86, microcode:
> Correct sysdev_add error path)
> 
> Cc: stable@vger.kernel.org
> Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Greg Kroah-Hartman]

On Thu, Apr 12, 2012 at 06:34:22PM +0200, Borislav Petkov wrote:
> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> Date: Thu, 12 Apr 2012 16:51:57 +0200
> Subject: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for supported AMD CPUs
> 
> Exit early when there's no support for a particular CPU family. Also,
> fixup the "no support for this CPU vendor" to be issued only when the
> driver is attempted to be loaded on an unsupported vendor.
> 
> Cc: stable@vger.kernel.org
> Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
> [Boris: added a commit msg because Andreas is lazy]
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[tip:x86/urgent] x86, microcode: Fix sysfs warning during module unload on unsupported CPUs

[tip-bot for Andreas Herrmann]

Commit-ID:  a956bd6f8583326b18348ab1452b4686778f785d
Gitweb:     http://git.kernel.org/tip/a956bd6f8583326b18348ab1452b4686778f785d
Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
AuthorDate: Thu, 12 Apr 2012 16:48:01 +0200
Committer:  Borislav Petkov <borislav.petkov@amd.com>
CommitDate: Fri, 13 Apr 2012 11:49:06 +0200

x86, microcode: Fix sysfs warning during module unload on unsupported CPUs

Loading the microcode driver on an unsupported CPU and subsequently
unloading the driver causes

 WARNING: at fs/sysfs/group.c:138 mc_device_remove+0x5f/0x70 [microcode]()
 Hardware name: 01972NG
 sysfs group ffffffffa00013d0 not found for kobject 'cpu0'
 Modules linked in: snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel btusb snd_hda_codec bluetooth thinkpad_acpi rfkill microcode(-) [last unloaded: cfg80211]
 Pid: 4560, comm: modprobe Not tainted 3.4.0-rc2-00002-g258f742 #5
 Call Trace:
  [<ffffffff8103113b>] ? warn_slowpath_common+0x7b/0xc0
  [<ffffffff81031235>] ? warn_slowpath_fmt+0x45/0x50
  [<ffffffff81120e74>] ? sysfs_remove_group+0x34/0x120
  [<ffffffffa00000ef>] ? mc_device_remove+0x5f/0x70 [microcode]
  [<ffffffff81331eb9>] ? subsys_interface_unregister+0x69/0xa0
  [<ffffffff81563526>] ? mutex_lock+0x16/0x40
  [<ffffffffa0000c3e>] ? microcode_exit+0x50/0x92 [microcode]
  [<ffffffff8107051d>] ? sys_delete_module+0x16d/0x260
  [<ffffffff810a0065>] ? wait_iff_congested+0x45/0x110
  [<ffffffff815656af>] ? page_fault+0x1f/0x30
  [<ffffffff81565ba2>] ? system_call_fastpath+0x16/0x1b

on recent kernels.

This is due to commit 8a25a2fd126c ("cpu: convert 'cpu' and
'machinecheck' sysdev_class to a regular subsystem") which renders
commit 6c53cbfced04 ("x86, microcode: Correct sysdev_add error path")
useless.

See http://marc.info/?l=linux-kernel&m=133416246406478

Avoid above warning by restoring the old driver behaviour before
6c53cbfced04 ("x86, microcode: Correct sysdev_add error path").

Cc: stable@vger.kernel.org
Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: http://lkml.kernel.org/r/20120411163849.GE4794@alberich.amd.com
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/microcode_core.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 87a0f86..d389e74 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -419,10 +419,8 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
 	if (err)
 		return err;
 
-	if (microcode_init_cpu(cpu) == UCODE_ERROR) {
-		sysfs_remove_group(&dev->kobj, &mc_attr_group);
+	if (microcode_init_cpu(cpu) == UCODE_ERROR)
 		return -EINVAL;
-	}
 
 	return err;
 }

[tip:x86/urgent] x86, microcode: Ensure that module is only loaded on supported AMD CPUs

[tip-bot for Andreas Herrmann]

Commit-ID:  283c1f2558ef4a4411fe908364b15b73b6ab44cf
Gitweb:     http://git.kernel.org/tip/283c1f2558ef4a4411fe908364b15b73b6ab44cf
Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
AuthorDate: Thu, 12 Apr 2012 16:51:57 +0200
Committer:  Borislav Petkov <borislav.petkov@amd.com>
CommitDate: Fri, 13 Apr 2012 11:51:05 +0200

x86, microcode: Ensure that module is only loaded on supported AMD CPUs

Exit early when there's no support for a particular CPU family. Also,
fixup the "no support for this CPU vendor" to be issued only when the
driver is attempted to be loaded on an unsupported vendor.

Cc: stable@vger.kernel.org
Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: http://lkml.kernel.org/r/20120411163849.GE4794@alberich.amd.com
[Boris: add a commit msg because Andreas is lazy]
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/microcode_amd.c  |   12 +++++++-----
 arch/x86/kernel/microcode_core.c |    6 +++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 73465aa..8a2ce8f 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -82,11 +82,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
-	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
-		pr_warning("CPU%d: family %d not supported\n", cpu, c->x86);
-		return -1;
-	}
-
 	csig->rev = c->microcode;
 	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
 
@@ -380,6 +375,13 @@ static struct microcode_ops microcode_amd_ops = {
 
 struct microcode_ops * __init init_amd_microcode(void)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
+		pr_warning("AMD CPU family 0x%x not supported\n", c->x86);
+		return NULL;
+	}
+
 	patch = (void *)get_zeroed_page(GFP_KERNEL);
 	if (!patch)
 		return NULL;
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index d389e74..c9bda6d 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -526,11 +526,11 @@ static int __init microcode_init(void)
 		microcode_ops = init_intel_microcode();
 	else if (c->x86_vendor == X86_VENDOR_AMD)
 		microcode_ops = init_amd_microcode();
-
-	if (!microcode_ops) {
+	else
 		pr_err("no support for this CPU vendor\n");
+
+	if (!microcode_ops)
 		return -ENODEV;
-	}
 
 	microcode_pdev = platform_device_register_simple("microcode", -1,
 							 NULL, 0);

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Srivatsa S. Bhat]

On 04/12/2012 10:04 PM, Borislav Petkov wrote:

> From: Andreas Herrmann <andreas.herrmann3@amd.com>
> Date: Thu, 12 Apr 2012 16:51:57 +0200
> Subject: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for supported AMD CPUs
> 
> Exit early when there's no support for a particular CPU family. Also,
> fixup the "no support for this CPU vendor" to be issued only when the
> driver is attempted to be loaded on an unsupported vendor.
> 
> Cc: stable@vger.kernel.org
> Cc: Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
> [Boris: added a commit msg because Andreas is lazy]
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---


I guess a similar change in intel microcode driver would also be
worthwhile, right? If so, then the below patch might be useful:

----------------->

From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: x86, microcode: Ensure that module is only loaded on supported Intel CPUs

Exit early when there's no support for a particular CPU family.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/kernel/microcode_intel.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 3ca42d0..0327e2b 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -147,12 +147,6 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 
 	memset(csig, 0, sizeof(*csig));
 
-	if (c->x86_vendor != X86_VENDOR_INTEL || c->x86 < 6 ||
-	    cpu_has(c, X86_FEATURE_IA64)) {
-		pr_err("CPU%d not a capable Intel processor\n", cpu_num);
-		return -1;
-	}
-
 	csig->sig = cpuid_eax(0x00000001);
 
 	if ((c->x86_model >= 5) || (c->x86 > 6)) {
@@ -463,6 +457,14 @@ static struct microcode_ops microcode_intel_ops = {
 
 struct microcode_ops * __init init_intel_microcode(void)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	if (c->x86_vendor != X86_VENDOR_INTEL || c->x86 < 6 ||
+	    cpu_has(c, X86_FEATURE_IA64)) {
+		pr_err("Intel CPU family 0x%x not supported\n", c->x86);
+		return NULL;
+	}
+
 	return &microcode_intel_ops;
 }
 

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Borislav Petkov]

On Mon, Apr 16, 2012 at 02:12:00PM +0530, Srivatsa S. Bhat wrote:
> I guess a similar change in intel microcode driver would also be
> worthwhile, right? If so, then the below patch might be useful:

AFAICT, you'd need to test this on a box which is not supported (family
check below). Or you can tweak the family check so that it doesn't
apply and run it on a test box of yours to see whether the module still
unloads properly in the error case (and doesn't remain loaded as in
our case due to something in sysfs remaining registered although it
shouldn't).

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Srivatsa S. Bhat]

On 04/16/2012 07:13 PM, Borislav Petkov wrote:

> On Mon, Apr 16, 2012 at 02:12:00PM +0530, Srivatsa S. Bhat wrote:
>> I guess a similar change in intel microcode driver would also be
>> worthwhile, right? If so, then the below patch might be useful:
> 
> AFAICT, you'd need to test this on a box which is not supported (family
> check below). Or you can tweak the family check so that it doesn't
> apply and run it on a test box of yours to see whether the module still
> unloads properly in the error case (and doesn't remain loaded as in
> our case due to something in sysfs remaining registered although it
> shouldn't).
> 


Judging by the nature of the code itself, its evident that this problem
is not restricted to AMD alone. In any case, I went ahead and did the
(tweaked) test that you suggested and here is the result:

1. dmesg logs :
[   21.912451] microcode: CPU0 not a capable Intel processor
[   21.916028] microcode: CPU1 not a capable Intel processor
[   21.934624] microcode: CPU2 not a capable Intel processor
[   21.940276] microcode: CPU3 not a capable Intel processor
[   21.946023] microcode: CPU4 not a capable Intel processor
[   21.951678] microcode: CPU5 not a capable Intel processor
[   21.957326] microcode: CPU6 not a capable Intel processor
[   21.962935] microcode: CPU7 not a capable Intel processor
[   21.968611] microcode: CPU8 not a capable Intel processor
[   21.974212] microcode: CPU9 not a capable Intel processor
[   21.979842] microcode: CPU10 not a capable Intel processor
[   21.985577] microcode: CPU11 not a capable Intel processor
[   21.991323] microcode: CPU12 not a capable Intel processor
[   21.997055] microcode: CPU13 not a capable Intel processor
[   22.002774] microcode: CPU14 not a capable Intel processor
[   22.008460] microcode: CPU15 not a capable Intel processor
[   22.014600] microcode: Microcode Update Driver: v2.00 <tigran@aivazian.fsnet.co.uk>, Peter Oruba

2. lsmod | grep microcode
microcode             117749  0

3. rmmod microcode

4. dmesg logs:
[   64.297638] ------------[ cut here ]------------
[   64.302332] WARNING: at fs/sysfs/group.c:142 sysfs_remove_group+0xd9/0xe0()
[   64.309446] Hardware name: IBM System x -[7870C4Q]-
[   64.309449] sysfs group ffffffffa03a5110 not found for kobject 'cpu0'
[   64.309451] Modules linked in: ipv6 cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf fuse loop dm_mod coretemp crc32c_intel shpchp microcode(-) cdc_ether usbnet ioatdma bnx2 mii serio_raw pcspkr pci_hotplug i2c_i801 i7core_edac i2c_core dca edac_core iTCO_wdt iTCO_vendor_support tpm_tis tpm sg tpm_bios rtc_cmos button uhci_hcd ehci_hcd usbcore usb_common sd_mod crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase scsi_transport_sas scsi_mod thermal thermal_sys hwmon
[   64.309499] Pid: 6529, comm: rmmod Not tainted 3.4.0-rc3-intelucodebroken-0.0.0.28.36b5ec9-default #2
[   64.309501] Call Trace:
[   64.309506]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
[   64.309510]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
[   64.309516]  [<ffffffff8103dd2a>] warn_slowpath_common+0x7a/0xb0
[   64.309522]  [<ffffffff8103de01>] warn_slowpath_fmt+0x41/0x50
[   64.309526]  [<ffffffff811f87b6>] ? sysfs_get_dirent+0x56/0x80
[   64.309531]  [<ffffffff811fac69>] sysfs_remove_group+0xd9/0xe0
[   64.309538]  [<ffffffffa03a2118>] mc_device_remove+0x78/0xa0 [microcode]
[   64.309545]  [<ffffffff8134b219>] subsys_interface_unregister+0x89/0xd0
[   64.309553]  [<ffffffffa03a357a>] microcode_exit+0x5a/0xa4 [microcode]
[   64.309559]  [<ffffffff810b0c8a>] sys_delete_module+0x16a/0x2b0
[   64.309565]  [<ffffffff810a408d>] ? trace_hardirqs_on_caller+0x12d/0x1b0
[   64.309573]  [<ffffffff814b36f9>] system_call_fastpath+0x16/0x1b
[   64.309576] ---[ end trace 748ddf8b4218f820 ]---


The above warning was repeated for each cpu, of course.
 
Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Borislav Petkov]

On Tue, Apr 17, 2012 at 07:41:22PM +0530, Srivatsa S. Bhat wrote:
> On 04/16/2012 07:13 PM, Borislav Petkov wrote:
> 
> > On Mon, Apr 16, 2012 at 02:12:00PM +0530, Srivatsa S. Bhat wrote:
> >> I guess a similar change in intel microcode driver would also be
> >> worthwhile, right? If so, then the below patch might be useful:
> > 
> > AFAICT, you'd need to test this on a box which is not supported (family
> > check below). Or you can tweak the family check so that it doesn't
> > apply and run it on a test box of yours to see whether the module still
> > unloads properly in the error case (and doesn't remain loaded as in
> > our case due to something in sysfs remaining registered although it
> > shouldn't).
> > 
> Judging by the nature of the code itself, its evident that this problem
> is not restricted to AMD alone. In any case, I went ahead and did the
> (tweaked) test that you suggested and here is the result:
> 
> 1. dmesg logs :
> [   21.912451] microcode: CPU0 not a capable Intel processor
> [   21.916028] microcode: CPU1 not a capable Intel processor
> [   21.934624] microcode: CPU2 not a capable Intel processor
> [   21.940276] microcode: CPU3 not a capable Intel processor
> [   21.946023] microcode: CPU4 not a capable Intel processor
> [   21.951678] microcode: CPU5 not a capable Intel processor
> [   21.957326] microcode: CPU6 not a capable Intel processor
> [   21.962935] microcode: CPU7 not a capable Intel processor
> [   21.968611] microcode: CPU8 not a capable Intel processor
> [   21.974212] microcode: CPU9 not a capable Intel processor
> [   21.979842] microcode: CPU10 not a capable Intel processor
> [   21.985577] microcode: CPU11 not a capable Intel processor
> [   21.991323] microcode: CPU12 not a capable Intel processor
> [   21.997055] microcode: CPU13 not a capable Intel processor
> [   22.002774] microcode: CPU14 not a capable Intel processor
> [   22.008460] microcode: CPU15 not a capable Intel processor
> [   22.014600] microcode: Microcode Update Driver: v2.00 <tigran@aivazian.fsnet.co.uk>, Peter Oruba
> 
> 2. lsmod | grep microcode
> microcode             117749  0
> 
> 3. rmmod microcode
> 
> 4. dmesg logs:
> [   64.297638] ------------[ cut here ]------------
> [   64.302332] WARNING: at fs/sysfs/group.c:142 sysfs_remove_group+0xd9/0xe0()
> [   64.309446] Hardware name: IBM System x -[7870C4Q]-
> [   64.309449] sysfs group ffffffffa03a5110 not found for kobject 'cpu0'
> [   64.309451] Modules linked in: ipv6 cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf fuse loop dm_mod coretemp crc32c_intel shpchp microcode(-) cdc_ether usbnet ioatdma bnx2 mii serio_raw pcspkr pci_hotplug i2c_i801 i7core_edac i2c_core dca edac_core iTCO_wdt iTCO_vendor_support tpm_tis tpm sg tpm_bios rtc_cmos button uhci_hcd ehci_hcd usbcore usb_common sd_mod crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase scsi_transport_sas scsi_mod thermal thermal_sys hwmon
> [   64.309499] Pid: 6529, comm: rmmod Not tainted 3.4.0-rc3-intelucodebroken-0.0.0.28.36b5ec9-default #2
> [   64.309501] Call Trace:
> [   64.309506]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
> [   64.309510]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
> [   64.309516]  [<ffffffff8103dd2a>] warn_slowpath_common+0x7a/0xb0
> [   64.309522]  [<ffffffff8103de01>] warn_slowpath_fmt+0x41/0x50
> [   64.309526]  [<ffffffff811f87b6>] ? sysfs_get_dirent+0x56/0x80
> [   64.309531]  [<ffffffff811fac69>] sysfs_remove_group+0xd9/0xe0
> [   64.309538]  [<ffffffffa03a2118>] mc_device_remove+0x78/0xa0 [microcode]
> [   64.309545]  [<ffffffff8134b219>] subsys_interface_unregister+0x89/0xd0
> [   64.309553]  [<ffffffffa03a357a>] microcode_exit+0x5a/0xa4 [microcode]
> [   64.309559]  [<ffffffff810b0c8a>] sys_delete_module+0x16a/0x2b0
> [   64.309565]  [<ffffffff810a408d>] ? trace_hardirqs_on_caller+0x12d/0x1b0
> [   64.309573]  [<ffffffff814b36f9>] system_call_fastpath+0x16/0x1b
> [   64.309576] ---[ end trace 748ddf8b4218f820 ]---
> 
> 
> The above warning was repeated for each cpu, of course.

Right, this is the warning we're seeing too. Applying your patch fixes
it, I s'pose? If so, maybe x86 people would pick it up.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Gene Heskett]

On Tuesday, April 17, 2012, Borislav Petkov wrote:
>On Tue, Apr 17, 2012 at 07:41:22PM +0530, Srivatsa S. Bhat wrote:
>> On 04/16/2012 07:13 PM, Borislav Petkov wrote:
>> > On Mon, Apr 16, 2012 at 02:12:00PM +0530, Srivatsa S. Bhat wrote:
>> >> I guess a similar change in intel microcode driver would also be
>> > 
>> >> worthwhile, right? If so, then the below patch might be useful:
>> > AFAICT, you'd need to test this on a box which is not supported
>> > (family check below). Or you can tweak the family check so that it
>> > doesn't apply and run it on a test box of yours to see whether the
>> > module still unloads properly in the error case (and doesn't remain
>> > loaded as in our case due to something in sysfs remaining registered
>> > although it shouldn't).
>> 
>> Judging by the nature of the code itself, its evident that this problem
>> is not restricted to AMD alone. In any case, I went ahead and did the
>> (tweaked) test that you suggested and here is the result:
>> 
>> 1. dmesg logs :
>> [   21.912451] microcode: CPU0 not a capable Intel processor
>> [   21.916028] microcode: CPU1 not a capable Intel processor
>> [   21.934624] microcode: CPU2 not a capable Intel processor
>> [   21.940276] microcode: CPU3 not a capable Intel processor
>> [   21.946023] microcode: CPU4 not a capable Intel processor
>> [   21.951678] microcode: CPU5 not a capable Intel processor
>> [   21.957326] microcode: CPU6 not a capable Intel processor
>> [   21.962935] microcode: CPU7 not a capable Intel processor
>> [   21.968611] microcode: CPU8 not a capable Intel processor
>> [   21.974212] microcode: CPU9 not a capable Intel processor
>> [   21.979842] microcode: CPU10 not a capable Intel processor
>> [   21.985577] microcode: CPU11 not a capable Intel processor
>> [   21.991323] microcode: CPU12 not a capable Intel processor
>> [   21.997055] microcode: CPU13 not a capable Intel processor
>> [   22.002774] microcode: CPU14 not a capable Intel processor
>> [   22.008460] microcode: CPU15 not a capable Intel processor
>> [   22.014600] microcode: Microcode Update Driver: v2.00
>> <tigran@aivazian.fsnet.co.uk>, Peter Oruba
>> 
>> 2. lsmod | grep microcode
>> microcode             117749  0
>> 
>> 3. rmmod microcode
>> 
>> 4. dmesg logs:
>> [   64.297638] ------------[ cut here ]------------
>> [   64.302332] WARNING: at fs/sysfs/group.c:142
>> sysfs_remove_group+0xd9/0xe0() [   64.309446] Hardware name: IBM
>> System x -[7870C4Q]-
>> [   64.309449] sysfs group ffffffffa03a5110 not found for kobject
>> 'cpu0' [   64.309451] Modules linked in: ipv6 cpufreq_conservative
>> cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf fuse loop
>> dm_mod coretemp crc32c_intel shpchp microcode(-) cdc_ether usbnet
>> ioatdma bnx2 mii serio_raw pcspkr pci_hotplug i2c_i801 i7core_edac
>> i2c_core dca edac_core iTCO_wdt iTCO_vendor_support tpm_tis tpm sg
>> tpm_bios rtc_cmos button uhci_hcd ehci_hcd usbcore usb_common sd_mod
>> crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase
>> scsi_transport_sas scsi_mod thermal thermal_sys hwmon [   64.309499]
>> Pid: 6529, comm: rmmod Not tainted
>> 3.4.0-rc3-intelucodebroken-0.0.0.28.36b5ec9-default #2 [   64.309501]
>> Call Trace:
>> [   64.309506]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
>> [   64.309510]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
>> [   64.309516]  [<ffffffff8103dd2a>] warn_slowpath_common+0x7a/0xb0
>> [   64.309522]  [<ffffffff8103de01>] warn_slowpath_fmt+0x41/0x50
>> [   64.309526]  [<ffffffff811f87b6>] ? sysfs_get_dirent+0x56/0x80
>> [   64.309531]  [<ffffffff811fac69>] sysfs_remove_group+0xd9/0xe0
>> [   64.309538]  [<ffffffffa03a2118>] mc_device_remove+0x78/0xa0
>> [microcode] [   64.309545]  [<ffffffff8134b219>]
>> subsys_interface_unregister+0x89/0xd0 [   64.309553] 
>> [<ffffffffa03a357a>] microcode_exit+0x5a/0xa4 [microcode] [  
>> 64.309559]  [<ffffffff810b0c8a>] sys_delete_module+0x16a/0x2b0 [  
>> 64.309565]  [<ffffffff810a408d>] ?
>> trace_hardirqs_on_caller+0x12d/0x1b0 [   64.309573] 
>> [<ffffffff814b36f9>] system_call_fastpath+0x16/0x1b [   64.309576]
>> ---[ end trace 748ddf8b4218f820 ]---
>> 
>> 
>> The above warning was repeated for each cpu, of course.
>
>Right, this is the warning we're seeing too. Applying your patch fixes
>it, I s'pose? If so, maybe x86 people would pick it up.

I do not think the above has anything to do with the current version 
shipping in some distro's, which is trying to find the intel code on 
kernels >2.6.39 when running on AMD stuff, so that script in /etc/init.d 
needs attention too.  I have nuked about half of that script in my local 
copy so that it does install AMD code on AMD processors.  The errors I was 
seeing were only the lack of its ability to find the Intel code and does 
not generate any output resembling the above.

That test in that script for a minimum minor kernel version to determine 
which microcode to apply seems totally bogus to me anyway, but I am not 
privy to enough cpu info to write the correct test.

Cheers, Gene
-- 
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
My web page: <http://coyoteden.dyndns-free.com:85/gene>
"Most of us, when all is said and done, like what we like and make up 
reasons 
for it afterwards."
-- Soren F. Petersen

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Kay Sievers]

2012/4/17 Gene Heskett <gene.heskett@gmail.com>:

> I do not think the above has anything to do with the current version
> shipping in some distro's, which is trying to find the intel code on
> kernels >2.6.39 when running on AMD stuff, so that script in /etc/init.d
> needs attention too.  I have nuked about half of that script in my local
> copy so that it does install AMD code on AMD processors.  The errors I was
> seeing were only the lack of its ability to find the Intel code and does
> not generate any output resembling the above.

Just a note: the intel microcode is loaded by a kernel-initiated
firmware request these days. There is no (broken) init script on
recent systems, to (rather mindlessly) fiddle around in userspace with
kernel internals.

Kay

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Gene Heskett]

On Tuesday, April 17, 2012, Kay Sievers wrote:
>2012/4/17 Gene Heskett <gene.heskett@gmail.com>:
>> I do not think the above has anything to do with the current version
>> shipping in some distro's, which is trying to find the intel code on
>> kernels >2.6.39 when running on AMD stuff, so that script in
>> /etc/init.d needs attention too. آ I have nuked about half of that
>> script in my local copy so that it does install AMD code on AMD
>> processors. آ The errors I was seeing were only the lack of its
>> ability to find the Intel code and does not generate any output
>> resembling the above.
>
>Just a note: the intel microcode is loaded by a kernel-initiated
>firmware request these days. There is no (broken) init script on
>recent systems, to (rather mindlessly) fiddle around in userspace with
>kernel internals.
>
>Kay

That has to be a good deal in general, but it leaves us AMD users swinging 
in a strong breeze. That microcode loading was in the kernel until sometime 
in the 2.6.20ish time frame, I can recall scanning dmesg's where it was 
loaded by about line 75 in past history.  Now its just an afterthought. :(

Thanks Kay.

Cheers, Gene
-- 
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
My web page: <http://coyoteden.dyndns-free.com:85/gene>
The American nation in the sixth ward is a fine people; they love the
eagle -- on the back of a dollar.
		-- Finlay Peter Dunne

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Srivatsa S. Bhat]

On 04/17/2012 08:20 PM, Borislav Petkov wrote:

> On Tue, Apr 17, 2012 at 07:41:22PM +0530, Srivatsa S. Bhat wrote:
>> On 04/16/2012 07:13 PM, Borislav Petkov wrote:
>>
>>> On Mon, Apr 16, 2012 at 02:12:00PM +0530, Srivatsa S. Bhat wrote:
>>>> I guess a similar change in intel microcode driver would also be
>>>> worthwhile, right? If so, then the below patch might be useful:
>>>
>>> AFAICT, you'd need to test this on a box which is not supported (family
>>> check below). Or you can tweak the family check so that it doesn't
>>> apply and run it on a test box of yours to see whether the module still
>>> unloads properly in the error case (and doesn't remain loaded as in
>>> our case due to something in sysfs remaining registered although it
>>> shouldn't).
>>>
>> Judging by the nature of the code itself, its evident that this problem
>> is not restricted to AMD alone. In any case, I went ahead and did the
>> (tweaked) test that you suggested and here is the result:
>>
>> 1. dmesg logs :
>> [   21.912451] microcode: CPU0 not a capable Intel processor
>> [   21.916028] microcode: CPU1 not a capable Intel processor
>> [   21.934624] microcode: CPU2 not a capable Intel processor
>> [   21.940276] microcode: CPU3 not a capable Intel processor
>> [   21.946023] microcode: CPU4 not a capable Intel processor
>> [   21.951678] microcode: CPU5 not a capable Intel processor
>> [   21.957326] microcode: CPU6 not a capable Intel processor
>> [   21.962935] microcode: CPU7 not a capable Intel processor
>> [   21.968611] microcode: CPU8 not a capable Intel processor
>> [   21.974212] microcode: CPU9 not a capable Intel processor
>> [   21.979842] microcode: CPU10 not a capable Intel processor
>> [   21.985577] microcode: CPU11 not a capable Intel processor
>> [   21.991323] microcode: CPU12 not a capable Intel processor
>> [   21.997055] microcode: CPU13 not a capable Intel processor
>> [   22.002774] microcode: CPU14 not a capable Intel processor
>> [   22.008460] microcode: CPU15 not a capable Intel processor
>> [   22.014600] microcode: Microcode Update Driver: v2.00 <tigran@aivazian.fsnet.co.uk>, Peter Oruba
>>
>> 2. lsmod | grep microcode
>> microcode             117749  0
>>
>> 3. rmmod microcode
>>
>> 4. dmesg logs:
>> [   64.297638] ------------[ cut here ]------------
>> [   64.302332] WARNING: at fs/sysfs/group.c:142 sysfs_remove_group+0xd9/0xe0()
>> [   64.309446] Hardware name: IBM System x -[7870C4Q]-
>> [   64.309449] sysfs group ffffffffa03a5110 not found for kobject 'cpu0'
>> [   64.309451] Modules linked in: ipv6 cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf fuse loop dm_mod coretemp crc32c_intel shpchp microcode(-) cdc_ether usbnet ioatdma bnx2 mii serio_raw pcspkr pci_hotplug i2c_i801 i7core_edac i2c_core dca edac_core iTCO_wdt iTCO_vendor_support tpm_tis tpm sg tpm_bios rtc_cmos button uhci_hcd ehci_hcd usbcore usb_common sd_mod crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase scsi_transport_sas scsi_mod thermal thermal_sys hwmon
>> [   64.309499] Pid: 6529, comm: rmmod Not tainted 3.4.0-rc3-intelucodebroken-0.0.0.28.36b5ec9-default #2
>> [   64.309501] Call Trace:
>> [   64.309506]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
>> [   64.309510]  [<ffffffff811fac69>] ? sysfs_remove_group+0xd9/0xe0
>> [   64.309516]  [<ffffffff8103dd2a>] warn_slowpath_common+0x7a/0xb0
>> [   64.309522]  [<ffffffff8103de01>] warn_slowpath_fmt+0x41/0x50
>> [   64.309526]  [<ffffffff811f87b6>] ? sysfs_get_dirent+0x56/0x80
>> [   64.309531]  [<ffffffff811fac69>] sysfs_remove_group+0xd9/0xe0
>> [   64.309538]  [<ffffffffa03a2118>] mc_device_remove+0x78/0xa0 [microcode]
>> [   64.309545]  [<ffffffff8134b219>] subsys_interface_unregister+0x89/0xd0
>> [   64.309553]  [<ffffffffa03a357a>] microcode_exit+0x5a/0xa4 [microcode]
>> [   64.309559]  [<ffffffff810b0c8a>] sys_delete_module+0x16a/0x2b0
>> [   64.309565]  [<ffffffff810a408d>] ? trace_hardirqs_on_caller+0x12d/0x1b0
>> [   64.309573]  [<ffffffff814b36f9>] system_call_fastpath+0x16/0x1b
>> [   64.309576] ---[ end trace 748ddf8b4218f820 ]---
>>
>>
>> The above warning was repeated for each cpu, of course.
> 
> Right, this is the warning we're seeing too. Applying your patch fixes
> it, I s'pose?


Of course.. Sorry I didn't mention it explicitly.
Strictly speaking, in the patch series by Andreas,
 patch 1/2: gets rid of the warning,
 patch 2/2: ensures that the microcode module doesn't remain loaded
 when unsupported cpus are detected - but it does it only for AMD cpus.

My patch is very much like 2/2 except that it is for the Intel cpus -
if the underlying cpus are unsupported, then it ensures that the module
loading fails. [The warning is already silenced by patch 1/2].

> If so, maybe x86 people would pick it up.
> 


Ok, thanks!

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Borislav Petkov]

On Tue, Apr 17, 2012 at 01:30:35PM -0400, Gene Heskett wrote:
> On Tuesday, April 17, 2012, Kay Sievers wrote:
> >2012/4/17 Gene Heskett <gene.heskett@gmail.com>:
> >> I do not think the above has anything to do with the current version
> >> shipping in some distro's, which is trying to find the intel code on
> >> kernels >2.6.39 when running on AMD stuff, so that script in
> >> /etc/init.d needs attention too. آ I have nuked about half of that
> >> script in my local copy so that it does install AMD code on AMD
> >> processors. آ The errors I was seeing were only the lack of its
> >> ability to find the Intel code and does not generate any output
> >> resembling the above.
> >
> >Just a note: the intel microcode is loaded by a kernel-initiated
> >firmware request these days. There is no (broken) init script on
> >recent systems, to (rather mindlessly) fiddle around in userspace with
> >kernel internals.
> >
> >Kay
> 
> That has to be a good deal in general, but it leaves us AMD users swinging 
> in a strong breeze. That microcode loading was in the kernel until sometime 
> in the 2.6.20ish time frame, I can recall scanning dmesg's where it was 
> loaded by about line 75 in past history.  Now its just an afterthought. :(

Well, I don't know what your script does but it should simply

$ modprobe microcode

and the module should figure it out itself which microcode to load,
AFAICT. 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Henrique de Moraes Holschuh]

On Tue, 17 Apr 2012, Kay Sievers wrote:
> 2012/4/17 Gene Heskett <gene.heskett@gmail.com>:
> > I do not think the above has anything to do with the current version
> > shipping in some distro's, which is trying to find the intel code on
> > kernels >2.6.39 when running on AMD stuff, so that script in /etc/init.d
> > needs attention too.  I have nuked about half of that script in my local
> > copy so that it does install AMD code on AMD processors.  The errors I was
> > seeing were only the lack of its ability to find the Intel code and does
> > not generate any output resembling the above.
> 
> Just a note: the intel microcode is loaded by a kernel-initiated
> firmware request these days. There is no (broken) init script on

Is it?  Which distros have deployed the required userspace and switched to
Intel binary microcode already?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/2] x86, microcode: Ensure that module is only loaded for

[Kay Sievers]

On Sun, Apr 22, 2012 at 04:55, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 17 Apr 2012, Kay Sievers wrote:

>> Just a note: the intel microcode is loaded by a kernel-initiated
>> firmware request these days. There is no (broken) init script on
>
> Is it?  Which distros have deployed the required userspace and switched to
> Intel binary microcode already?

The tool is here:
  http://pkgs.fedoraproject.org/repo/pkgs/microcode_ctl/intel-microcode2ucode.c/0efc5f6c74a4d7e61ca22683c93c98cf/intel-microcode2ucode.c

It's at least used here:
  http://pkgs.fedoraproject.org/gitweb/?p=microcode_ctl.git;a=tree
  https://build.opensuse.org/package/files?package=microcode_ctl&project=Base%3ASystem
  http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/sys-apps/microcode-data/files/

Kay

[tip:x86/urgent] x86/microcode: Ensure that module is only loaded on supported Intel CPUs

[tip-bot for Srivatsa S. Bhat]

Commit-ID:  7164b3f5e5461b58baa82e82544ca5af71af287b
Gitweb:     http://git.kernel.org/tip/7164b3f5e5461b58baa82e82544ca5af71af287b
Author:     Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
AuthorDate: Mon, 16 Apr 2012 14:12:00 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 7 May 2012 14:37:14 +0200

x86/microcode: Ensure that module is only loaded on supported Intel CPUs

Exit early when there's no support for a particular CPU family.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: Dave Jones <davej@redhat.com>
Cc: tigran@aivazian.fsnet.co.uk
Cc: Borislav Petkov <borislav.petkov@amd.com>
Link: http://lkml.kernel.org/r/4F8BDB58.6070007@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/microcode_intel.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 3ca42d0..0327e2b 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -147,12 +147,6 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 
 	memset(csig, 0, sizeof(*csig));
 
-	if (c->x86_vendor != X86_VENDOR_INTEL || c->x86 < 6 ||
-	    cpu_has(c, X86_FEATURE_IA64)) {
-		pr_err("CPU%d not a capable Intel processor\n", cpu_num);
-		return -1;
-	}
-
 	csig->sig = cpuid_eax(0x00000001);
 
 	if ((c->x86_model >= 5) || (c->x86 > 6)) {
@@ -463,6 +457,14 @@ static struct microcode_ops microcode_intel_ops = {
 
 struct microcode_ops * __init init_intel_microcode(void)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	if (c->x86_vendor != X86_VENDOR_INTEL || c->x86 < 6 ||
+	    cpu_has(c, X86_FEATURE_IA64)) {
+		pr_err("Intel CPU family 0x%x not supported\n", c->x86);
+		return NULL;
+	}
+
 	return &microcode_intel_ops;
 }