Messages in this thread Patch in this message | | | Date | Tue, 31 Jan 2023 08:51:25 -0800 | From | Ashok Raj <> | Subject | Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if microcode loading was successful |
| |
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();
| |