Bug 1411232 - microcode.service fails to load due to broken quoting in ExecStart line
Summary: microcode.service fails to load due to broken quoting in ExecStart line
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: microcode_ctl
Version: 7.3
Hardware: Unspecified
OS: Unspecified
high
medium
Target Milestone: rc
: ---
Assignee: Petr Oros
QA Contact: Rachel Sibley
URL:
Whiteboard:
: 1411718 1412173 1425109 (view as bug list)
Depends On: 1402512
Blocks: 1381646 1412187
TreeView+ depends on / blocked
 
Reported: 2017-01-09 08:34 UTC by Michal Schmidt
Modified: 2020-04-15 15:03 UTC (History)
39 users (show)

Fixed In Version: microcode_ctl-2.1-20.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1412187 (view as bug list)
Environment:
Last Closed: 2017-08-01 20:18:17 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Knowledge Base (Solution) 2851331 None None None 2017-01-09 09:36:47 UTC
Red Hat Product Errata RHEA-2017:1851 normal SHIPPED_LIVE microcode_ctl bug fix and enhancement update 2017-08-01 18:22:41 UTC

Description Michal Schmidt 2017-01-09 08:34:27 UTC
Description of problem:

The latest update of microcode_ctl broke the microcode.service systemd unit. In the journal I can see systemd reports errors about loading the unit.

Version-Release number of selected component (if applicable):
The bug is new in:
microcode_ctl  2:2.1-16.1.el7_3.x86_64
(it was not in 2:2.1-16.el7.x86_64)

How reproducible:
always

Steps to Reproduce:
  journalctl -b -p err
or:
  systemctl status microcode

Actual results:
led 09 09:23:19 localhost systemd[1]: [/usr/lib/systemd/system/microcode.service:10] Trailing garbage, ignoring.
led 09 09:23:19 localhost systemd[1]: microcode.service lacks both ExecStart= and ExecStop= setting. Refusing.

● microcode.service - Load CPU microcode update
   Loaded: error (Reason: Invalid argument)
   Active: inactive (dead)

Expected results:
No errors from systemd about microcode.service.
The unit must load successfully:
● microcode.service - Load CPU microcode update
   Loaded: loaded (/usr/lib/systemd/system/microcode.service; enabled; vendor preset: enabled)
   ...

Additional info:
The simplest fix could be to replace the outer quotes with single apostrophes:

-ExecStart=/usr/bin/bash -c "grep -l GenuineIntel /proc/cpuinfo | xargs grep -l "model.*79" > /dev/null || echo 1 > /sys/devices/system/cpu/microcode/reload"
+ExecStart=/usr/bin/bash -c 'grep -l GenuineIntel /proc/cpuinfo | xargs grep -l "model.*79" > /dev/null || echo 1 > /sys/devices/system/cpu/microcode/reload'

Comment 1 Michal Schmidt 2017-01-09 08:40:55 UTC
The buggy update was released in 7.3.z as RHBA-2017:0028-04 microcode_ctl bug fix update. So the fix needs to go to 7.3.z as well.

Comment 6 Stanislav Kozina 2017-01-10 12:00:09 UTC
This bug should not affect functionality as the expected CPU microcode is still loaded by the rpm %post script:

 49         grep -l GenuineIntel /proc/cpuinfo | xargs grep -l "model.*79" > /dev/null || \
 50         echo 1 > /sys/devices/system/cpu/microcode/reload

However the failed systemd service still needs to be fixed.

Comment 7 Stanislav Kozina 2017-01-10 12:00:28 UTC
*** Bug 1411718 has been marked as a duplicate of this bug. ***

Comment 9 Harald Reindl 2017-01-10 13:00:34 UTC
How does the post script help since it is only executed at install time but not at boot - frankly how can it be that people working that sloppy not doing a "systemctl daemob-reload" on the machine they edit unit files and look in their damned syslog after doing so? 

99% of all errors in systemd-units in the past years on RHEL and Fedora would never had made it into any testing rpm and so not to stable updates if people just read their logs as i do after every update

Comment 11 Stanislav Kozina 2017-01-11 08:41:00 UTC
Harald,
On the next boot microcode is updated from the dracut image. This is done because, in some cases, microcode cannot be updated on a running system.

Comment 12 Petr Oros 2017-01-11 13:11:48 UTC
*** Bug 1412173 has been marked as a duplicate of this bug. ***

Comment 24 Trevor Hemsley 2017-01-17 20:35:43 UTC
That grep looks a bit suspect to me. That will almost certainly match on an i7-4790k from the 

model name	: Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz

and that has 

model		: 60

Comment 25 Trevor Hemsley 2017-01-17 23:58:00 UTC
Would be better as:

ExecStart=/usr/bin/bash -c 'grep -l GenuineIntel /proc/cpuinfo | xargs grep -l -P "model\t.*79$" > /dev/null || echo 1 > /sys/devices/system/cpu/microcode/reload'

Comment 26 Petr Oros 2017-01-18 12:23:31 UTC
Good catch,
Fixed in last respin microcode_ctl-2.1-20.el7

Comment 28 Tuomo Soini 2017-01-24 13:25:42 UTC
Exactly same regexp problem is in %post script.

Comment 30 Carsten Grohmann 2017-01-26 14:15:28 UTC
If the intention is to update only Intel CPUs, then the suggested statement does not work, because xargs returns an error if it can't read anything from stdin.

Example with an AMD CPU:

# cat /proc/cpuinfo  | head -n 2
processor : 0
vendor_id : AuthenticAMD

# grep -l GenuineIntel /proc/cpuinfo | xargs grep -l -P "model\t.*79$" > /dev/null || echo 1
1

Comment 31 Petr Oros 2017-01-26 14:32:05 UTC
No, this is for update all cpu's but expression exclude from update intel cpu model 79.

Comment 32 Carsten Grohmann 2017-01-26 15:08:39 UTC
Would it be possible to use a better readable statement or to add a commend in top of the statement to show the intention (all but model 79)?

Comment 33 tbsky 2017-01-28 15:01:27 UTC
hi:
   May I ask since microcode already applied via dracut image, why we need systemd service to load it again?

Comment 34 Stanislav Kozina 2017-01-30 14:59:01 UTC
tbsky,
In most cases the systemd service isn't necessary, that's why this bug is really not critical. It's there mostly for safety reasons, if you build your own initramfs and forget to include the updated microcode file, the service might help in updating the microcode during boot. Obviously this isn't sufficient solution because in some cases the microcode needs to be updated before glibc is loaded, that is from initramfs. You should really make sure the initramfs contains the proper microcode. Thanks!

Comment 35 Jeff Bastian 2017-02-20 15:15:25 UTC
*** Bug 1425109 has been marked as a duplicate of this bug. ***

Comment 36 Frank Dietrich 2017-03-02 11:28:20 UTC
Based on the comments #25, #31 and #32 I propose to change it as

ExecStart=/usr/bin/bash -c 'grep -l GenuineIntel /proc/cpuinfo | xargs grep -q "model[[:blank:]]*: 79$" || echo 1 > /sys/devices/system/cpu/microcode/reload'

this would execute 
   echo 1 > /sys/devices/system/cpu/microcode/reload

for any
 - non GenuineIntel CPU
 - GenuineIntel CPU, except the one with "model    : 79"

Comment 37 Harald Reindl 2017-03-02 11:37:17 UTC
frankly that is all crap and belongs into a script / binary which is called by "ExecStart" - when something in a systemd-unit starts with "/usr/bin/bash -c" someone is absuing systemd-units

however, how many months will it take until that garbage is fixed?

the is still no update for the broken microcode_ctl-2.1-16.1.el7_3.x86_64 which would not have mad it to any package if the person who wrote it would have *one time* called "systemctl daemon-reload" and looked in the syslogs, besides that there obviously no QA exists at all

Comment 38 WRSomsky 2017-03-03 02:59:51 UTC
I have to agree w/ Harald that this is one of the ungodliest kludges I have ever seen.  

First, yes, it should have been put in an auxiliary script rather than being shoehorned in with 'bash -c'.  But even more so, what is that horrible shell construct?  Grep for 'GenuineIntel' in /proc/cpuinfo and return the name of the file if it matches, then xargs that into grepping for 'model 79' in whatever file might have had a match from the first grep (gee, might it have been /proc/cpuinfo?), tossing away any matches found into /dev/null and then if that found nothing, then do the echo...

If you're going to try to use a one-(long-)line shell construct (and including robustifying the grep patterns, which Frank started, but didn't carry far enough) how about something much more straightforward like this:

  grep -q '^vendor_id[[:blank:]]*: GenuineIntel$' /proc/cpuinfo \
    && grep -q '^model[[:blank:]]*: 79$' /proc/cpuinfo \
      || echo 1 > /sys/devices/system/cpu/microcode/reload

Note that I'm not worrying here how to further quote and escape this to be able to wrap it in 'bash -c' and place that in an ExecStart= line -- all the more reason to put whatever one decides to use inside an auxiliary script file!

Comment 39 Stanislav Kozina 2017-03-03 07:49:45 UTC
Hello,

The fix for RHEL-7.3.z has been released yesterday under bz1412187. Please review following Errata with the details:
https://rhn.redhat.com/errata/RHBA-2017-0374.html

Thanks.

Comment 41 Rachel Sibley 2017-05-05 18:52:58 UTC
ALL TESTS PASSED

Verified as fixed in microcode_ctl-2.1-20.el7, the service now loads without errors

Before
======================
~]# rpm -qa microcode_ctl
microcode_ctl-2.1-16.1.el7_3.x86_64

~]# systemctl status microcode -l
● microcode.service - Load CPU microcode update
   Loaded: error (Reason: Invalid argument)
   Active: inactive (dead) since Fri 2017-05-05 18:24:00 EDT; 3h 41min left
 Main PID: 725 (code=exited, status=0/SUCCESS)

May 05 18:23:59 localhost.localdomain systemd[1]: Starting Load CPU microcode update...
May 05 18:24:00 localhost.localdomain systemd[1]: Started Load CPU microcode update.
May 05 14:41:06 dell-per210-01.khw.lab.eng.bos.redhat.com systemd[1]: [/usr/lib/systemd/system/microcode.service:10] Trailing garbage, ignoring.
May 05 14:41:06 dell-per210-01.khw.lab.eng.bos.redhat.com systemd[1]: microcode.service lacks both ExecStart= and ExecStop= setting. Refusing.
~]# 

After
======================
~]# yum update microcode_ctl-2.1-20.el7.x86_64.rpm

~]# rpm -qa microcode_ctl
microcode_ctl-2.1-20.el7.x86_64

~]# systemctl restart microcode

~]# systemctl status microcode -l
● microcode.service - Load CPU microcode update
   Loaded: loaded (/usr/lib/systemd/system/microcode.service; enabled; vendor preset: enabled)
   Active: inactive (dead) since Fri 2017-05-05 14:44:18 EDT; 30s ago
  Process: 29163 ExecStart=/usr/bin/bash -c grep -l GenuineIntel /proc/cpuinfo | xargs grep -l -E "model[[:space:]]*: 79$" > /dev/null || echo 1 > /sys/devices/system/cpu/microcode/reload (code=exited, status=0/SUCCESS)
 Main PID: 29163 (code=exited, status=0/SUCCESS)

May 05 14:44:18 dell-per210-01.khw.lab.eng.bos.redhat.com systemd[1]: Starting Load CPU microcode update...
May 05 14:44:18 dell-per210-01.khw.lab.eng.bos.redhat.com systemd[1]: Started Load CPU microcode update.
~]#

Comment 42 errata-xmlrpc 2017-08-01 20:18:17 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:1851


Note You need to log in before you can comment on or make changes to this bug.