Currently when late loading is aborted due to check_online_cpu(), kernel
still ends up tainting the kernel.
Taint only when microcode loading was successful.
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
Cc: LKML <[email protected]>
Cc: x86 <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Alison Schofield <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: Thomas Gleixner (Intel) <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Stefan Talpalaru <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Peter Zilstra (Intel) <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Martin Pohlack <[email protected]>
---
v1->v2: (Thomas)
- Remove unnecessary assignment of ret that's being overwritten.
- Taint kernel only of loading was successful
---
arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 61d57d9b93ee..1c6831b8b244 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
enum ucode_state tmp_ret = UCODE_OK;
int bsp = boot_cpu_data.cpu_index;
unsigned long val;
- ssize_t ret = 0;
+ int load_ret = -1;
+ ssize_t ret;
ret = kstrtoul(buf, 0, &val);
if (ret)
@@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
goto put;
tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
- if (tmp_ret != UCODE_NEW)
+ if (tmp_ret != UCODE_NEW) {
+ ret = size;
goto put;
+ }
mutex_lock(µcode_mutex);
- ret = microcode_reload_late();
+ load_ret = microcode_reload_late();
mutex_unlock(µcode_mutex);
put:
cpus_read_unlock();
- if (ret == 0)
+ /*
+ * Taint only when loading was successful
+ */
+ if (load_ret == 0) {
ret = size;
-
- add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+ add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+ pr_warn("Microcode late loading tainted the kernel\n");
+ }
return ret;
}
--
2.37.2
On Mon, Jan 30, 2023 at 01:39:47PM -0800, Ashok Raj wrote:
> arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
Why all this hoopla and unrelated changes?
Why don't you simply hoist the call to ->request_microcode_fw outside of
the locked region as it doesn't have to be there and then do the usual
pattern?
---
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 14a2280fdcd2..23f4f22df581 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -481,28 +481,28 @@ static ssize_t reload_store(struct device *dev,
if (val != 1)
return size;
+ tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
+ if (tmp_ret != UCODE_NEW)
+ return ret;
+
cpus_read_lock();
ret = check_online_cpus();
if (ret)
- goto put;
-
- tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
- if (tmp_ret != UCODE_NEW)
- goto put;
+ goto unlock;
mutex_lock(µcode_mutex);
ret = microcode_reload_late();
mutex_unlock(µcode_mutex);
-put:
- cpus_read_unlock();
-
if (ret == 0)
ret = size;
add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+unlock:
+ cpus_read_unlock();
+
return ret;
}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 2023/1/31 5:39, Ashok Raj wrote:
> Currently when late loading is aborted due to check_online_cpu(), kernel
> still ends up tainting the kernel.
>
> Taint only when microcode loading was successful.
>
> Suggested-by: Thomas Gleixner <[email protected]>
> Signed-off-by: Ashok Raj <[email protected]>
> Cc: LKML <[email protected]>
> Cc: x86 <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Alison Schofield <[email protected]>
> Cc: Reinette Chatre <[email protected]>
> Cc: Thomas Gleixner (Intel) <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Stefan Talpalaru <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Peter Zilstra (Intel) <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Cc: Boris Ostrovsky <[email protected]>
> Cc: Martin Pohlack <[email protected]>
> ---
> v1->v2: (Thomas)
> - Remove unnecessary assignment of ret that's being overwritten.
> - Taint kernel only of loading was successful
> ---
> arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 61d57d9b93ee..1c6831b8b244 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
> enum ucode_state tmp_ret = UCODE_OK;
> int bsp = boot_cpu_data.cpu_index;
> unsigned long val;
> - ssize_t ret = 0;
> + int load_ret = -1;
> + ssize_t ret;
>
> ret = kstrtoul(buf, 0, &val);
> if (ret)
> @@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
> goto put;
>
> tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
> - if (tmp_ret != UCODE_NEW)
> + if (tmp_ret != UCODE_NEW) {
> + ret = size;
> goto put;
> + }
>
> mutex_lock(µcode_mutex);
> - ret = microcode_reload_late();
> + load_ret = microcode_reload_late();
> mutex_unlock(µcode_mutex);
>
> put:
> cpus_read_unlock();
>
> - if (ret == 0)
> + /*
> + * Taint only when loading was successful
> + */
> + if (load_ret == 0) {
> ret = size;
What about if loading was not successful(load_ret != 0)?
ret has no chance to be returned as size here and we'll run into the
endless update?
Thanks,
-Aubrey
> -
> - add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> + pr_warn("Microcode late loading tainted the kernel\n");
> + }
>
> return ret;
> }
On Tue, Jan 31, 2023 at 08:17:25PM +0800, Li, Aubrey wrote:
> On 2023/1/31 5:39, Ashok Raj wrote:
> > Currently when late loading is aborted due to check_online_cpu(), kernel
> > still ends up tainting the kernel.
> >
> > Taint only when microcode loading was successful.
> >
[snip]
> > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> > index 61d57d9b93ee..1c6831b8b244 100644
> > --- a/arch/x86/kernel/cpu/microcode/core.c
> > +++ b/arch/x86/kernel/cpu/microcode/core.c
> > @@ -472,7 +472,8 @@ static ssize_t reload_store(struct device *dev,
> > enum ucode_state tmp_ret = UCODE_OK;
> > int bsp = boot_cpu_data.cpu_index;
> > unsigned long val;
> > - ssize_t ret = 0;
> > + int load_ret = -1;
> > + ssize_t ret;
> > ret = kstrtoul(buf, 0, &val);
> > if (ret)
> > @@ -488,20 +489,26 @@ static ssize_t reload_store(struct device *dev,
> > goto put;
> > tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
> > - if (tmp_ret != UCODE_NEW)
> > + if (tmp_ret != UCODE_NEW) {
> > + ret = size;
> > goto put;
> > + }
> > mutex_lock(µcode_mutex);
> > - ret = microcode_reload_late();
> > + load_ret = microcode_reload_late();
> > mutex_unlock(µcode_mutex);
> > put:
> > cpus_read_unlock();
> > - if (ret == 0)
> > + /*
> > + * Taint only when loading was successful
> > + */
> > + if (load_ret == 0) {
> > ret = size;
>
> What about if loading was not successful(load_ret != 0)?
> ret has no chance to be returned as size here and we'll run into the endless
> update?
Good catch, we'll need to make that some meaningful return code to stop the
endless wait.
On Tue, Jan 31, 2023 at 12:50:44PM +0100, Borislav Petkov wrote:
> On Mon, Jan 30, 2023 at 01:39:47PM -0800, Ashok Raj wrote:
> > arch/x86/kernel/cpu/microcode/core.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
>
> Why all this hoopla and unrelated changes?
>
> Why don't you simply hoist the call to ->request_microcode_fw outside of
> the locked region as it doesn't have to be there and then do the usual
> pattern?
Makes total sense, and seems to make the code more readable. Thanks!
Just some minor changes below.
remove ret = 0 during initialization since its cleared right below. (tglx)
Some more below, updated patch at the end.
I have tested with the modified patch below.
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 14a2280fdcd2..23f4f22df581 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -481,28 +481,28 @@ static ssize_t reload_store(struct device *dev,
> if (val != 1)
> return size;
>
> + tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
> + if (tmp_ret != UCODE_NEW)
> + return ret;
> +
> cpus_read_lock();
>
> ret = check_online_cpus();
> if (ret)
> - goto put;
> -
> - tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
> - if (tmp_ret != UCODE_NEW)
> - goto put;
> + goto unlock;
Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
endlessly waiting for write to complete. (As Aubrey pointed out)
>
> mutex_lock(µcode_mutex);
> ret = microcode_reload_late();
I think its safe to leave ret as is, since microcode_reload_late() only
returns -1, or 0.
> mutex_unlock(µcode_mutex);
>
> -put:
> - cpus_read_unlock();
> -
> if (ret == 0)
> ret = size;
>
> add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
Pull this into the ret == 0, so taint only if the update was successful?
And add a message so its not silent?
>
> +unlock:
> + cpus_read_unlock();
> +
> return ret;
> }
>
>
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 94d942c1bf2c..550b7c566311 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -472,7 +472,7 @@ static ssize_t reload_store(struct device *dev,
enum ucode_state tmp_ret = UCODE_OK;
int bsp = boot_cpu_data.cpu_index;
unsigned long val;
- ssize_t ret = 0;
+ ssize_t ret;
ret = kstrtoul(buf, 0, &val);
if (ret)
@@ -483,7 +483,7 @@ static ssize_t reload_store(struct device *dev,
tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
if (tmp_ret != UCODE_NEW)
- return ret;
+ return (tmp_ret == UCODE_ERROR ? -EINVAL : size);
cpus_read_lock();
@@ -495,10 +495,11 @@ static ssize_t reload_store(struct device *dev,
ret = microcode_reload_late();
mutex_unlock(µcode_mutex);
- if (ret == 0)
+ if (ret == 0) {
ret = size;
-
- add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+ add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+ pr_warn("Microcode late loading tainted the kernel\n");
+ }
unlock:
cpus_read_unlock();
On Tue, Jan 31, 2023 at 08:51:25AM -0800, Ashok Raj wrote:
> remove ret = 0 during initialization since its cleared right below. (tglx)
Sure.
> Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
> endlessly waiting for write to complete. (As Aubrey pointed out)
Then do:
tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
if (tmp_ret != UCODE_NEW)
return size;
to signal what it is. It certainly ain't an error if it doesn't find new
microcode.
> I think its safe to leave ret as is, since microcode_reload_late() only
> returns -1, or 0.
No it doesn't. Hint: stop_machine_cpuslocked().
> Pull this into the ret == 0, so taint only if the update was successful?
Ok.
> And add a message so its not silent?
You'd add a printk for every possible operation, wouldn't you?
See, the world doesn't revolve around microcode loading. If that thing
fails, then someone has done a bad job at the CPU vendor testing,
provided the code does the right thing.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Jan 31, 2023 at 09:20:37PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 08:51:25AM -0800, Ashok Raj wrote:
> > remove ret = 0 during initialization since its cleared right below. (tglx)
>
> Sure.
>
> > Need to set ret explicitly to either -EINVAL, or size. Otherwise it will be
> > endlessly waiting for write to complete. (As Aubrey pointed out)
>
> Then do:
>
> tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev);
> if (tmp_ret != UCODE_NEW)
> return size;
>
> to signal what it is. It certainly ain't an error if it doesn't find new
> microcode.
It's not an error, only when request_microcode() returns UCODE_ERROR, should
it return -EINVAL, if its UCODE_NFOUND, or otherwise the code should treat
as success.
The diff I attached was: https://lore.kernel.org/lkml/[email protected]/
if (tmp_ret != UCODE_NEW)
- return ret;
+ return (tmp_ret == UCODE_ERROR ? -EINVAL : size);
Does the above look fine?
>
> > I think its safe to leave ret as is, since microcode_reload_late() only
> > returns -1, or 0.
>
> No it doesn't. Hint: stop_machine_cpuslocked().
>
> > Pull this into the ret == 0, so taint only if the update was successful?
>
> Ok.
>
> > And add a message so its not silent?
>
> You'd add a printk for every possible operation, wouldn't you?
:-) Not like that.. But looking through most of the cases that does
add_taint() either have some print, or there some big spalt message around
it.
This shouldn't be noisy, but if you think this isn't needed, it can go
away.
>
> See, the world doesn't revolve around microcode loading. If that thing
> fails, then someone has done a bad job at the CPU vendor testing,
> provided the code does the right thing.
>
When it fails due to current_rev < min_rev, Isn't it good to add indication
to user space that it didn't succeed? Thomas wanted these return codes, so
someone scripting can get a status after an attempt to load.
Otherwise agree, it shouldn't generally fail.
On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> It's not an error, only when request_microcode() returns UCODE_ERROR, should
> it return -EINVAL,
So looking at ->request_microcode_fw(), it looks like we return
UCODE_ERROR when something with parsing the blob has gone wrong. So I
guess we can return something more fitting here to state that we failed
while parsing the microcode blob from userspace: it is corrupted,
truncated, what not.
Looking at the error codes, this:
#define ELIBBAD 80 /* Accessing a corrupted shared library */
seems fitting as it has "corrupted" blob in the definition. EBADF sounds
fitting too. In any case, it should be a distinct error value which
hints at what goes wrong.
> This shouldn't be noisy, but if you think this isn't needed, it can go
> away.
I think all this preemptive development - it might make sense so let's
do it - needs to stop. If there's an *actual* real use and need for it
sure, but let's issue a printk just because is not one of them.
> When it fails due to current_rev < min_rev, Isn't it good to add indication
> to user space that it didn't succeed? Thomas wanted these return codes, so
> someone scripting can get a status after an attempt to load.
Return codes: yes. Random, flaky, potentially overwritten in the dmesg
ring buffer error strings - nope. Soon someone will come along and say,
"hey, don't touch those printk formats - my tool parses them and it'll
break if you do." Yeah, right.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
>
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
>
> Looking at the error codes, this:
>
> #define ELIBBAD 80 /* Accessing a corrupted shared library */
>
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.
Perfect!. In the next respin, I can update this to EBADF.
Later patches when we do minrev checks, or if the current rev is higher
than the one in the directory, what error would be fitting? Currently we do
EINVAL, I can't find a suitable one for that, if you have any good
suggestions that would be awesome.
>
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
>
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
>
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
>
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.
No arguments there.
I'll drop the pr_warn() before taint as you suggest.
Appears we have good direction for patches 1-3.
Cheers,
Ashok
Hi Boris,
While reworking I thought while at this, there is a chance to fix other
things.
On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
>
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
>
> Looking at the error codes, this:
>
> #define ELIBBAD 80 /* Accessing a corrupted shared library */
>
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.
Along the same lines, when check_online_cpu() should we return -EBUSY which
would distinguish between -EINVAL vs something different?
And we have a pr_err() to indicate not all CPUs are online. I suppose this
can be dropped in the same patch to fix return code? Since the error code
should be indicative of the problem?
pr_err("Not all CPUs online, aborting microcode update.\n");
>
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
>
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
>
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
>
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 01, 2023 at 01:47:58PM -0800, Ashok Raj wrote:
> While reworking I thought while at this, there is a chance to fix other
> things.
Your call. The more you're "fixing" other things, the longer it takes
for your minrev thing.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 01, 2023 at 11:06:05PM +0100, Borislav Petkov wrote:
> On Wed, Feb 01, 2023 at 01:47:58PM -0800, Ashok Raj wrote:
> > While reworking I thought while at this, there is a chance to fix other
> > things.
>
> Your call. The more you're "fixing" other things, the longer it takes
> for your minrev thing.
My original patch for minrev was 4 patches :-)
But when Thomas finds all the other cleanups, it turned into 9. And now
stuck in review..
I expected the change from apply_microcode() -> collect_cpu_info() to
be straight forward..
On Wed, Feb 01, 2023 at 02:19:43PM -0800, Ashok Raj wrote:
> I expected the change from apply_microcode() -> collect_cpu_info() to
> be straight forward..
I expect a lot of things things too but reality is completely different.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette