Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AMD Microcode Check #285

Closed
wants to merge 2 commits into from
Closed

Conversation

fritz-fritz
Copy link
Contributor

As discussed in #249, the default behavior for AMD was changed with pull request #226 and as a result, CPUs that do not have microcode patches (because they're up to date) were leading to NRM_UNKNOWN resulting in "Failed to check for processor microcode upgrades." which is inaccurate.

This sets the $vars{AVAIL} to 0 if there are microcode patches to scan and none apply. While this doesn't guarantee microcode is up to date or from a valid source, it restores the previous behavior while accounting for the edge case presented in the previous pull request.

This adds an elif to set AMD ucode checks to current if Microcode was found but none apply to the CPU. This fixes the logic to revert back to previous and expected behavior for AMD with the added benefit of keeping an NRM_UNKNOWN default to trap if there is no microcode updates to scan.
Added a helpful debug line that outputs the $vars values to show what is CURRENT and AVAIL
@fritz-fritz
Copy link
Contributor Author

This should address some more of #274 but also might want to look into expected behavior for Intel as well?

@fritz-fritz
Copy link
Contributor Author

@liske would be great to have some feedback

@anarcat
Copy link
Contributor

anarcat commented Nov 15, 2023

@fritz-fritz why is this marked as draft, do you plan to get anything else done on this PR? otherwise i suggest just marking it as ready. i'll test this on our fleet now.

@fritz-fritz
Copy link
Contributor Author

@fritz-fritz why is this marked as draft

@anarcat mostly just to try and encourage feedback before merging. I believe I have implemented the expected behavior here but am willing to rework it if need be. When all looks good I’ll be happy to finalize the PR

@anarcat
Copy link
Contributor

anarcat commented Nov 15, 2023

the patch doesn't fix the issue for us. before the patch:

root@tb-build-03:~# needrestart -p
UNKN - Kernel: 6.1.0-13-amd64, Microcode: unknown, Services: 1 (!), Containers: none, Sessions: 1 (!!)|Kernel=0;0;;0;2 Services=1;;0;0 Containers=0;;0;0 Sessions=1;0;;0
Services:
- systemd-logind.service
Sessions:
- redacted @ user manager service

after the patch:

root@tb-build-03:~# dpkg -i needrestart_3.6-4+deb12u1_all.deb 
(Reading database ... 110354 files and directories currently installed.)
Preparing to unpack needrestart_3.6-4+deb12u1_all.deb ...
Unpacking needrestart (3.6-4+deb12u1) over (3.6-4) ...
Setting up needrestart (3.6-4+deb12u1) ...
Processing triggers for man-db (2.11.2-2) ...
root@tb-build-03:~# needrestart -p
UNKN - Kernel: 6.1.0-13-amd64, Microcode: unknown, Services: 1 (!), Containers: none, Sessions: 1 (!!)|Kernel=0;0;;0;2 Services=1;;0;0 Containers=0;;0;0 Sessions=1;0;;0
Services:
- systemd-logind.service
Sessions:
- redacted @ user manager service

@fritz-fritz
Copy link
Contributor Author

@anarcat can you try it with -w -v I’d like to get better visibility at what it sees when ran. Also to note, this PR doesn’t fix the intel microcode checking on AMD I have that in a separate PR. So that could be the issue?

@anarcat
Copy link
Contributor

anarcat commented Nov 15, 2023

-v always succeeds, that's the whole problem here. I think i found (and fixed) the root cause, try #288.

@anarcat
Copy link
Contributor

anarcat commented Nov 15, 2023

edit: moved this comment into the issue thread instead, in #249 (comment)

@anarcat
Copy link
Contributor

anarcat commented Jul 31, 2024

i think this should be closed in favor of #288 and #290, and in any case would require a rebase.

@fritz-fritz
Copy link
Contributor Author

@anarcat I admittedly have been away from this code for a while, but a quick review of your PRs and I think I agree.

Closing this PR.

@fritz-fritz fritz-fritz closed this Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants