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

[RFC] add ability to merge structured configs #42838

Open
wants to merge 4 commits into
base: master
from

Conversation

@teto
Copy link
Contributor

teto commented Jul 1, 2018

POC

Motivation for this change

Listed here #41103.
To sum up, I would like programs that require kernel modifications to be able to specify their kernel requirements. This might be disabled etc. but here is a proposition to show how it could look like

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

follow up of #41393

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Jul 5, 2018

@dezgeg thanks for reviewing/merging the structured config PR \o/

Being able to specify required config for packages was one of the motivation. I would love some feedback on how to best achieve this since the current way is not practical aka:

openvswitch = stdenv.mkDerivation {
....
  passthru.kernelExtraConfig = with import ../../../../lib/kernel.nix { inherit (kernel) version; lib = stdenv.lib; }; {
    OPENVSWITCH  = whenAtLeast "2.0" yes;
  };
}

I moved the version dependant filters whenAtLeast etc.. out of lib/kernel.nix so that lib/kernel.nix can be imported by lib/default.nix. This replaces with import ../../../../lib/kernel.nix { inherit (kernel) version; lib = stdenv.lib; }; with with import lib.kernel; but I am still stuck with
passthru.kernelExtraConfig = { OPENVSWITCH = yes; }

The problem is that if we just pass it like this

  my_lenovo_kernel = prev.linux_latest.override({
    structuredExtraConfig = prev.pkgs.openvswitch.passthru.kernelExtraConfig;
})

it erases conditions (e.g., whenAtLeast ) in common-config.nix such as
OPENVSWITCH = optional whenAtLeast "3.4" yes;

I wonder if it wouldn't be better to have packages define a list of config items like passthru.kernelExtraConfig = [ OPENVSWITCH ];and have the kernel config logic enable these the common-config.nix (strip the optional for instance) ?

To sum up, the best interface would be IMO to have in common-config.nix
OPENVSWITCH = optional whenAtLeast "3.4" no;
and find some way to turn this into OPENVSWITCH = whenAtLeast "3.4" yes; when building my_lenovo_kernel (cf previous example).

@teto teto force-pushed the teto:kernel_autoconf branch Jul 12, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Jul 14, 2018

I am experimenting a bit as I am trying to solve the initial issue. Without types, the code is hard to follow but what I've come up with is something similar to the module system so that hopefully merging
{ DEBUG = option yes; } and { DEBUG = yes; } returns sthg like { DEBUG = option yes; }.
So for instance yes is defined as yes = { answer = "y"; }. My fear is that I am recreating the module system so any hindsight would be nice.

lib/debug.nix Outdated
@@ -77,7 +77,7 @@ rec {
(modify depth snip x)) y;

/* A combination of `traceVal` and `traceSeq` */
traceValSeqFn = f: v: traceVal f (builtins.deepSeq v v);
traceValSeqFn = f: v: traceValFn f (builtins.deepSeq v v);

This comment has been minimized.

@Profpatsch

Profpatsch Jul 17, 2018

Member

Huh, I thought we merged that line already?

This comment has been minimized.

@teto

teto Jul 17, 2018

Contributor

it's not up to date. I've started modifying the code to adopt somewhat the module backend so it might be best not to review it now

@teto teto force-pushed the teto:kernel_autoconf branch 3 times, most recently from c16fd9e Jul 25, 2018

@teto teto changed the title [WIP] openvswitch: expose required kernel config [WIP] expose required kernel config Aug 1, 2018

@teto teto changed the title [WIP] expose required kernel config [WIP] add ability to merge structured configs Aug 1, 2018

@teto teto force-pushed the teto:kernel_autoconf branch Aug 1, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Aug 1, 2018

Thanks to the advice on https://discourse.nixos.org/t/use-lib-types-system-to-merge-attrsets-without-the-module-system/534/10, I managed to use the module system to merge structured configs. I renamed the pull request so that it focuses on this aspect only. It seems to work for basic things (linux_latest) but I am sure I've broken things in other places.

I tried to preserve the current api (option no; whenOlder "4.3" yes etc) but there are a few difficulties. Each kernel option is now a kernelItem submodule of the form https://github.com/NixOS/nixpkgs/pull/42838/files#diff-810dcf1e9edefca0d59fd0b90d11242bR30 .

One problem is for the "freeform" options like currently
MMC_BLOCK_MINORS = "32";
I could preprocess (slow) the structured config to convert the "32" into a submodule { answer = "32";}
so I preferred to write instead MMC_BLOCK_MINORS = freeform "32"; # equivalent to { answer = "32"} but I wonder if MMC_BLOCK_MINORS.answer = "32"; is better ?
It might be best (to allow for further checking) to distinguish between freeform answers (type = types.str) and tristate values (types.enum [ "y" "n" "m"])

I've converted the items of the form
B43_PCMCIA = option (whenOlder "4.4" yes);
to
B43_PCMCIA = whenOlder "4.4" (option yes);
with the whenOlder now being a mkIf. I suspect this can be a problem for some kernels. I will need to tackle that too.

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Aug 10, 2018

I went ahead and proceeded with the modifications:

  • extraStructuredConfig now expects a list of attributes. It makes the composition of the kernel config easier.
  • I've proceeded with distinguishing freeform options from tristate ones
  • will look for a structured config in kernelPatches too
  • one can now access the structuredConfig from a kernel via linux_test.configfile.structuredConfig in order to reinject it another kernel, no need to rewrite the config from scratch
  • I've defined the following merge strategies in case of conflict:
    -- freeform items must be equal or they conflict (mergeEqualOption)
    -- for tristate (y/m/n) entries, I use the mergeAnswer strategy which takes the best available value, "best" being defined by the user (by default "y" > "m" > "n", e.g. if one entry is both marked "y" and "n", "y" wins)
    -- if one item is both marked optional/mandatory, mandatory wins (mergeFalseByDefault)

Here is what I test with:

    linux_test = let 
      configStructured = with prev.lib.kernel; [ 


        # common-config.nix default is 32 

        { MMC_BLOCK_MINORS   = freeform "32"; } # same as default, won't trigger any error 
        # { MMC_BLOCK_MINORS   = freeform "64"; } # will trigger an error but the message is not great:
        # The option `settings.MMC_BLOCK_MINORS.freeform' has conflicting definitions, in `<unknown-file>' and `<unknown-file>'

        # mandatory should win by default
        { USB_DEBUG = option yes;}
        # { USB_DEBUG = yes;}

        # default for "8139TOO_PIO" is no
        { "8139TOO_PIO"  = yes; } # yes wins over no by default
      ];
    in
    prev.linux_latest.override {
      kernelPreferBuiltin = true;
      structuredExtraConfig = configStructured;
  };

You can then have a look at the generated config via
nix-build -A linux_latest.configfile ~/nixpkgs3 or look in nix repl for linux_test.configfile.structuredConfig

I am sure the code may be improved and there are some cases that might break. I would like to write some tests but not sure how to do.

@teto teto changed the title [WIP] add ability to merge structured configs [RFC] add ability to merge structured configs Aug 10, 2018

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Aug 10, 2018

Can lib/kernel.nix be moved out of lib? lib really shouldn't contain package-specific functions.

@dezgeg

This comment has been minimized.

Copy link
Contributor

dezgeg commented Aug 10, 2018

The kconfig language/mechanism is actually quite popular in the embedded community, for instance Buildroot, BusyBox, Crosstool-ng, OpenWRT, U-Boot all use it. Nothing of those that's currently packaged needs to care about the config file to the same precision as our kernel package does, however.

@Infinisil Infinisil self-requested a review Aug 11, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Aug 13, 2018

I would like packages to be able to specify kernel configurations, like enabling openvswitch should set OPENVSWITCH = yes; . To make yes/no available to packages I put them into lib/kernel.nix.
I can indeed move the rest ouf of lib/kernel.nix.

@teto teto force-pushed the teto:kernel_autoconf branch Aug 14, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Aug 14, 2018

I fixed the only trouble I had left when converting the optional configurations (some dropped optional items). I was able to compile 4_4/4_9/4_18/linux_hardened.

@edolstra I left only some aliases in https://github.com/NixOS/nixpkgs/blob/826e270c8d49f8876e48583c3f70672c4d250b11/lib/kernel.nix in kernel.nix for modules/packages to configure the kernel. But they could do without it and use the full config { optional = false; tristate = "y"; } instead so I can get rid of lib/kernel.nix entirely if you insist.

My only concern now is that I changed structuredExtraConfig from being a set (current master) to a list of set (so that lib.evalModules merges the configs). I wonder if instead one could not just pass an structuredExtraConfig=mkMerge[ {} {} ] to get the same results ?

@dtzWill

This comment has been minimized.

Copy link
Contributor

dtzWill commented Sep 6, 2018

(what's the status of this?)

@teto teto force-pushed the teto:kernel_autoconf branch Sep 6, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Sep 7, 2018

1/ I wanted to add tests for the strategies described in #42838 (comment) . Confirming these are the default strategies we want would help.
2/ I want to compose kernel configs (kvmConfig + ebpfConfig + mptcpConfig etc). This is already possible via passing extraStructuredConfig=[ kvmConfig ebpfConfig mptcpConfig ] but I would prefer extraStructuredConfig=mkMerge[ kvmConfig ebpfConfig mptcpConfig] but I haven't had the time to test it yet.
3/ I wanted to revert the conversion of mptcp and hardened configs to slim the PR a bit.

@dtzWill

This comment has been minimized.

Copy link
Contributor

dtzWill commented Sep 7, 2018

Okay! Looking forward to this!

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Sep 13, 2018

@dtzWill or anyone feel free to takeover :) (I won't be able to touch it for at least a month). I see you've packaged a few python kernel-related libraries ?! it would be cool if we could have the dependency tree between kernel config items.

@teto teto force-pushed the teto:kernel_autoconf branch to 37b72ec Oct 3, 2018

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Oct 3, 2018

I've addressed all the points in my previous message #42838 (comment)

From my side the PR is feature complete so now it's up to reviewers to decide what's wrong/false
To run the tests $ nix-build -A tests.kernel-config ~/nixpkgs3

A kernel I have in my overlay to test

    linux_test = let 
      # mininetConfigStructured = {};
      configStructured = with prev.lib.kernel; prev.lib.mkMerge [ 
    BPF = yes;
    BPF_SYSCALL = yes;
    VIRTIO            = mkForce yes;


      ];
    in
    prev.linux_latest.override {
      kernelPreferBuiltin = true;
      structuredExtraConfig = configStructured;
  };
@GrahamcOfBorg

This comment has been minimized.

Copy link

GrahamcOfBorg commented Oct 3, 2018

Success on aarch64-linux (full log)

Attempted: linux

Partial log (click to expand)

wrong ELF type
wrong ELF type
wrong ELF type
wrong ELF type
wrong ELF type
wrong ELF type
wrong ELF type
wrong ELF type
cannot find section .dynamic
/nix/store/088md53vh5i2r6yp63bvjf9dijdzrx7i-linux-4.14.73

@GrahamcOfBorg

This comment has been minimized.

Copy link

GrahamcOfBorg commented Oct 3, 2018

Timed out, unknown build status on x86_64-linux (full log)

Attempted: linux

Partial log (click to expand)

  CC [M]  drivers/staging/comedi/comedi_usb.o
  CC [M]  drivers/staging/comedi/comedi_fops.o
  CC [M]  drivers/net/wireless/marvell/mwifiex/11h.o
  CC [M]  drivers/net/wireless/marvell/mwifiex/tdls.o
  CC [M]  drivers/staging/comedi/range.o
  CC [M]  drivers/staging/comedi/drivers.o
  CC [M]  drivers/net/wireless/marvell/mwifiex/debugfs.o
  CC [M]  drivers/staging/comedi/comedi_buf.o
building of '/nix/store/5ra0rjjj28yhhajnzdqrks2mmmlwmj3n-linux-4.14.73.drv' timed out after 1800 seconds
error: build of '/nix/store/5ra0rjjj28yhhajnzdqrks2mmmlwmj3n-linux-4.14.73.drv' failed

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Nov 16, 2018

bump ?

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Jan 7, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-december/1711/8

@teto teto force-pushed the teto:kernel_autoconf branch from 37b72ec to 246e53a Jan 9, 2019

@teto

This comment has been minimized.

Copy link
Contributor

teto commented Jan 9, 2019

I rebased without problems on nixos-unstable. Then because there were some conflicts I favored my changes via git rebase master -Xtheirs and then I adapted the changes to hardened-config.nix between nixos-unstable and master manually, i.e. this list of changes.

diff --git a/pkgs/os-specific/linux/kernel/hardened-config.nix b/pkgs/os-specific/linux/kernel/hardened-config.nix
index 84d1dd8a378..4fadd447654 100644
--- a/pkgs/os-specific/linux/kernel/hardened-config.nix
+++ b/pkgs/os-specific/linux/kernel/hardened-config.nix
@@ -28,9 +28,9 @@ ${optionalString (stdenv.hostPlatform.platform.kernelArch == "x86_64") ''
   # Reduce attack surface by disabling various emulations
   IA32_EMULATION n
   X86_X32 n
-  ${optionalString (versionOlder version "4.17") ''
-    MODIFY_LDT_SYSCALL? n
-  ''}
+  # Note: this config depends on EXPERT y and so will not take effect, hence
+  # it is left "optional" for now.
+  MODIFY_LDT_SYSCALL? n
 
   VMAP_STACK y # Catch kernel stack overflows
 
@@ -52,18 +52,23 @@ ${optionalString (versionOlder version "4.11") ''
   DEBUG_SET_MODULE_RONX y
 ''}
 
-# Mark LSM hooks read-only after init.  Conflicts with SECURITY_SELINUX_DISABLE
-# (disabling SELinux at runtime); hence, SELinux can only be disabled at boot
-# via the selinux=0 boot parameter.
+# Mark LSM hooks read-only after init.  SECURITY_WRITABLE_HOOKS n
+# conflicts with SECURITY_SELINUX_DISABLE y; disabling the latter
+# implicitly marks LSM hooks read-only after init.
+#
+# SELinux can only be disabled at boot via selinux=0
+#
+# We set SECURITY_WRITABLE_HOOKS n primarily for documentation purposes; the
+# config builder fails to detect that it has indeed been unset.
 ${optionalString (versionAtLeast version "4.12") ''
   SECURITY_SELINUX_DISABLE n
-''}
-
-${optionalString ((versionAtLeast version "4.12") && (versionOlder version "4.17")) ''
-  SECURITY_WRITABLE_HOOKS n
+  SECURITY_WRITABLE_HOOKS? n
 ''}
 
 DEBUG_WX y # boot-time warning on RWX mappings
+${optionalString (versionAtLeast version "4.11") ''
+  STRICT_KERNEL_RWX y
+''}
 
 # Stricter /dev/mem
 STRICT_DEVMEM? y
@@ -84,7 +89,7 @@ ${optionalString (versionAtLeast version "4.13") ''
 # Perform usercopy bounds checking.
 HARDENED_USERCOPY y
 ${optionalString (versionAtLeast version "4.16") ''
-  HARDENED_USERCOPY_FALLBACK n
+  HARDENED_USERCOPY_FALLBACK n  # for full whitelist enforcement
 ''}
 
 # Randomize allocator freelists.
@@ -94,6 +99,9 @@ ${optionalString (versionAtLeast version "4.14") ''
   SLAB_FREELIST_HARDENED y
 ''}
 
+# Allow enabling slub/slab free poisoning with slub_debug=P
+SLUB_DEBUG y
+
 # Wipe higher-level memory allocations on free() with page_poison=1
 PAGE_POISONING y
 PAGE_POISONING_NO_SANITY y
@@ -103,17 +111,18 @@ PAGE_POISONING_ZERO y
 PANIC_ON_OOPS y
 PANIC_TIMEOUT -1
 
-${optionalString (versionOlder version "4.18") ''
-  GCC_PLUGINS y # Enable gcc plugin options
-  # Gather additional entropy at boot time for systems that may not have appropriate entropy sources.
-  GCC_PLUGIN_LATENT_ENTROPY y
-
-  ${optionalString (versionAtLeast version "4.11") ''
-    GCC_PLUGIN_STRUCTLEAK y # A port of the PaX structleak plugin
-  ''}
-  ${optionalString (versionAtLeast version "4.14") ''
-    GCC_PLUGIN_STRUCTLEAK_BYREF_ALL y # Also cover structs passed by address
-  ''}
+GCC_PLUGINS y # Enable gcc plugin options
+# Gather additional entropy at boot time for systems that may not have appropriate entropy sources.
+GCC_PLUGIN_LATENT_ENTROPY y
+
+${optionalString (versionAtLeast version "4.11") ''
+  GCC_PLUGIN_STRUCTLEAK y # A port of the PaX structleak plugin
+''}
+${optionalString (versionAtLeast version "4.14") ''
+  GCC_PLUGIN_STRUCTLEAK_BYREF_ALL y # Also cover structs passed by address
+''}
+${optionalString (versionAtLeast version "4.20") ''
+  GCC_PLUGIN_STACKLEAK y # A port of the PaX stackleak plugin
 ''}
 
 # Disable various dangerous settings

teto added some commits Oct 3, 2018

linux: ability to merge structured configs
This should make the composability of kernel configurations more straigthforward.

- now distinguish freeform options from tristate ones
- will look for a structured config in kernelPatches too
one can now access the structuredConfig from a kernel via linux_test.configfile.structuredConfig
in order to reinject it into another kernel, no need to rewrite the config from scratch

The following merge strategies are used in case of conflict:
-- freeform items must be equal or they conflict (mergeEqualOption)
-- for tristate (y/m/n) entries, I use the mergeAnswer strategy which takes the best available value, "best" being defined by the user (by default "y" > "m" > "n", e.g. if one entry is both marked "y" and "n", "y" wins)
-- if one item is both marked optional/mandatory, mandatory wins (mergeFalseByDefault)

@teto teto force-pushed the teto:kernel_autoconf branch from 246e53a to efa81f8 Jan 15, 2019

@teto teto referenced this pull request Jan 17, 2019

Open

Enable memory hotplug support #54095

4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment