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.

Sign up for GitHub

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

Jump to bottom

Upgrade kernel compiler to GCC 7. #34385

Closed
wants to merge 2 commits into from
Closed

Upgrade kernel compiler to GCC 7. #34385

wants to merge 2 commits into from

Conversation

clefru
Copy link
Contributor

@clefru clefru commented Jan 29, 2018 •
edited

Motivation for this change

Kernel spectre v2 hardening, see
#34383

Things done
  • Compiled and booted into.
  • Check that modinfo zfs shows vermagic: 4.14.15 SMP mod_unload retpoline.
  • Check the output of grep . /sys/devices/system/cpu/vulnerabilities/*:
/sys/devices/system/cpu/vulnerabilities/meltdown:Not affected
/sys/devices/system/cpu/vulnerabilities/spectre_v1:Vulnerable
/sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full AMD retpoline

This is only PR is only for discussion. I found the use of stdenvKernelCc a bit ugly, but couldn't find a more idiomatic expression.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Mic92
Mic92 reviewed Jan 29, 2018
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -13158,7 +13160,9 @@ with pkgs;

# A function to build a manually-configured kernel
linuxManualConfig = pkgs.buildLinux;
buildLinux = makeOverridable (callPackage ../os-specific/linux/kernel/manual-config.nix {});
buildLinux = makeOverridable (callPackage ../os-specific/linux/kernel/manual-config.nix {
buildPackages = buildPackages // { stdenv = overrideCC buildPackages.stdenv gcc7; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here, why gcc7 is required.
This makes it easier to remove this line later, when gcc7 becomes default or is superseded by newer versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

@fpletz
Copy link
Member

fpletz commented Jan 29, 2018

I'm still hoping we can make gcc7 the default really soon so we don't have to add this hack. But if we can't make it for 18.03, this will come in handy.

The other question is if we should backport this to 17.09. Then pushing this hack and gcc 7.3.0 to 17.09 is probably the easiest thing to do.

@clefru clefru force-pushed the gcc7-kernel branch 2 times, most recently from 957fdd0 to f5a3c19 Compare January 29, 2018 19:23
clefru
clefru commented Jan 29, 2018
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -12970,10 +12970,12 @@ with pkgs;
for a specific kernel. This function can then be called for
whatever kernel you're using. */

linuxPackagesFor = kernel: lib.makeExtensible (self: with self; {
linuxPackagesFor = kernel: let stdenvKernelCc = overrideCC stdenv kernel.buildCc;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found a better way to express that other than with introducing a "let" here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just stdenv = overrideCC pkgs.stdenv kernel.buildCc;?

@Mic92
Copy link
Member

Mic92 commented Jan 30, 2018

@fpletz if so, this should start soon. I expect breakage again with gcc7.

@clefru
Copy link
Contributor Author

clefru commented Jan 30, 2018

I ran the kernel-lts, kernel-latest and installer.simple tests. All succeed with a gcc7-compiled retpoline kernel. So, functionality-wise this PR can go in. If somebody has style comments, please add them.

Should we switch to gcc7 in general, only the last commit needs to be rolled-back. Using the kernel compiler for kernel module compilation should stay, as this is a sane thing to do.

clefru
clefru commented Feb 1, 2018
View reviewed changes
pkgs/top-level/all-packages.nix
@@ -12974,6 +12974,7 @@ with pkgs;
callPackage = newScope self;

inherit kernel;
stdenv = overrideCC pkgs.stdenv kernel.buildCc;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aszlig: I switched to the form you suggested. I also squashed commit 1+2 together as commit 2 became trivial.

@fpletz
Copy link
Member

fpletz commented Feb 1, 2018

@Mic92 There's already a branch and a hydra jobset. The efforts are going on for a while now and it looks pretty good: https://hydra.nixos.org/jobset/nixos/gcc-7

@clefru
Copy link
Contributor Author

clefru commented Feb 3, 2018

This is security problem. Can we get this merged soon? Then backport to 17.09. Then we can merge the gcc-7 branch into master, and rollback the last commit in master, leaving 17.09 with the backport.

@vcunat
Copy link
Member

vcunat commented Feb 3, 2018

Yes, I agree, do this now and revert later. The gcc7 branch will probably need some additional work (I see ~1.5k build regressions on Linux platforms ATM), and this PR is relatively cheap rebuild-wise.

Last call!

@vcunat
Copy link
Member

vcunat commented Feb 3, 2018

BTW, this will need nontrivial backporting work for 17.09, and we won't have gcc7 by default there.

@clefru
Copy link
Contributor Author

clefru commented Feb 5, 2018

At the risk of being annoying, this 1.severity: security PR is still open. It puts the kernel in the default state under gcc 7.3, which is retpoline enabled, which is what the kernel devs thought would be a good default despite the performance regressions.

@edolstra
Copy link
Member

edolstra commented Feb 5, 2018

IMHO we should not backport this to 17.09. Switching compilers is far outside of the scope of a "stable" branch.

@clefru clefru closed this Feb 5, 2018
@clefru clefru deleted the gcc7-kernel branch February 5, 2018 13:45
@vcunat
Copy link
Member

vcunat commented Feb 5, 2018

@edolstra: the intention was to backport compiler change only for kernel and its modules, not for all packages (I assume that's what you understood).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@aszlig aszlig aszlig left review comments

Assignees
No one assigned
Labels
1.severity: security 6.topic: kernel 10.rebuild-darwin: 0 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@clefru @fpletz @Mic92 @vcunat @edolstra @aszlig @GrahamcOfBorg