Comment 7 for bug 1742903

On 26 March 2018 at 13:07, Henrique de Moraes Holschuh
<email address hidden> wrote:
> On Mon, 26 Mar 2018, Dimitri John Ledkov wrote:
>> Something like:
>>
>> [[ `iucode_tool -s $(sudo iucode_tool --scan-system=2 2>&1 | sed
>> 's/.*\(0x.*\)$/\1/') -l /lib/firmware/intel-ucode/ | sed -n 's/.* rev
>> \(0x....\),.*/\1/p'` -eq `sudo cat
>> /sys/devices/system/cpu/cpu0/microcode/version` ]] || echo 'Trigger
>> reboot'
>>
>> should be good enough.... with bashism... as the kernel file is 0x1f,
>> yet the tool reports 0x001f.
>
> Bashisms aren't a problem, as long as the script declares to be a bash
> script. Those numbers have to be properly parsed, so using bash is a good
> solution. They are guaranteed to be 32-bit, too, which doesn't get in
> the way (bash math is 32-bit).
>
> The use of --scan-system=2 is a problem (slow when there are many CPUs,
> requires a kernel module to be loaded), and I don't understand why the
> whole dance to feed its output to iucode-tool -s. Looks like just
> "iucode_tool -S" would do what is required *and* handle systems in
> mixed stepping configurations better...
>

$(--scan-system) -> find the current processor signature e.g. 0x000306a9

Pass that as the selector, to the listing of all microcodes:

iucode_tool -s 0x000306a9 -l /lib/firmware/intel-ucode

Thus find me the available revision of ucode on disk.

Compare that with currently loaded revision of ucode, as reported via
kernel sysfs:
cat /sys/devices/system/cpu/cpu0/microcode/version

if the two do not match (on-disk vs loaded onto the cpu) -> request
reboot. This will bring up cases, of any missmatch, eg.
older/newer/dowgrade/upgrade.

Thus I am comparing runtime - what the cpu is reporting as loaded (via
sysfs), versus what I could have loaded from disk, if I reboot with a
fresh initramfs generated from the ucode on disk.

Note this will run only at postinst of intel-microcode package. Thus
if there is a false positive, this will only be triggered once, not at
like every call to `update-initramfs -u`. (for the cases of multiple
cpus / different cpu ids / etc).

I guess we can run above for every cpuX in the
/sys/devices/system/cpu/ directory, to catch ucode updates to all
cores.

>> There will be false negatives, and false positives, but overall should
>> be better than the current behavior.
>
> There are a lot of corner cases. Also, I am worried that it would have
> *both* false positives and false negatives. False positives are
> annoying and cause extra downtime (needlessly request a reboot), but
> false negatives are *bad* once people start trusting the package to
> request a reboot when one would be required.
>

I am not worried about that. One is expected to reboot approximately
every three week on Ubuntu anyway, due to full kernel updates.

> For example, one has to consider whether it should be checking the
> initrds as well or not. Note that while fortunately, the iucode_tool
> -tr loader option can read the initrd directly, the intel-microcode and
> iucode-tool packages have no idea where the initrd is, since this
> knowledge belongs to other layers (initramfs-tools/dracut, bootloader).
>

I am checking currently loaded, versus what is available on disk. I
believe this is sufficient, and there should be no need to parse
initrds.

Please note, this doesn't force a reboot, only shows generic
notification that reboot is suggested in MOTD / GUI popups, etc.

> Anyway, the script must learn to deal with the cases where the microcode
> update disappeared as well, or it will break badly.
>

Comparing ondisk with running, does this already. Empty does not equal
to something.

--
Regards,

Dimitri.