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

linux_5_2, linux_5_3: fixing nondeterminism #69222

Merged
merged 1 commit into from Oct 8, 2019

Conversation

d-goldin
Copy link
Contributor

@d-goldin d-goldin commented Sep 21, 2019

Motivation for this change

Addresses: #69100

In 5.2 kernel a new mechanism was introduced which embeds the kernel
headers in the kernel image and exposes them in procfs for simplified
use by userland tools.

It was introduced in
torvalds/linux@43d8ce9
and later modified a bit in
torvalds/linux@f7b101d

The archive containing the header files had nondeterminism through the
header files metadata - specifically mtime, but I also decided to
normalize some other aspects just in case.

In our default setup we currently compile this as a module, so to expose
the headers to test the functionality kheaders needs to be loaded.

Without this patch this particular nondeterminism was trivially reproducible locally. I ran a few compilations of 5.2 and 5.3 and the local difference is resolved with this.

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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @grahamc @thoughtpolice

@d-goldin
Copy link
Contributor Author

Regarding testing the result: I successfully booted into a locally built 5.3 and checked the contents of the kheaders archive in sysfs and all the dates are equal.

@thoughtpolice
Copy link
Member

Nice work! It would probably be worth it to submit this upstream, I'd think, in the long run; $SOURCE_DATE_EPOCH is relatively well established, but I think what we really want to do is make gen-kheaders respect KBUILD_BUILD_TIMESTAMP, as documented here, if we did that. Luckily we already set KBUILD_BUILD_TIMESTAMP in manual-config.nix to use $SOURCE_DATE_EPOCH directly, so if we did this, it should all work out I think.

@thoughtpolice thoughtpolice self-assigned this Sep 21, 2019
@d-goldin
Copy link
Contributor Author

d-goldin commented Sep 21, 2019

@thoughtpolice: Alright, I can give it a try. Sounds like a sensible thing to do.

@d-goldin
Copy link
Contributor Author

Gave it another run with 5.3, looks good!
Thanks for the pointers. It's getting late here, so I will look into possibly upstreaming tomorrow.

@d-goldin
Copy link
Contributor Author

I submitted a slightly modified patch covering the case of undefined timestamp for kernel builds not desiring reproducibility and got it a bit more in line with the kernel style guides; https://lkml.org/lkml/2019/9/22/69

I updated the patch in this PR to reflect the patch submitted upstream - while for the nixos case we wouldn't really care much for the case of timestamp being undefined, maybe there is some merit in keeping it more in line until/if it lands in mainline.

If reviewers are happy, I can squash them into one. Kept separately for clarity.

@Mic92
Copy link
Member

Mic92 commented Sep 22, 2019

I think we can wait for a few days to get the kernel maintainers feedback on this. Apart from that the changes looks good to me.

In 5.2 kernel a new mechanism was introduced which embeds the kernel
headers in the kernel image and exposes them in procfs for simplified
use by userland tools.

It was introduced in
torvalds/linux@43d8ce9
and later modified a bit in
torvalds/linux@f7b101d

The archive containing the header files had nondeterminism through the
header files metadata - specifically `mtime`, but I also decided to
normalize some other aspects just in case.

In our default setup we currently compile this as a module, so to expose
the headers to test the functionality `kheaders` needs to be loaded.

See https://lkml.org/lkml/2019/10/4/1036 and
https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/commit/?h=fixes&id=2cc99c9cdc8fde5e92e34f9655829449cebd3e00

I commented out the documentation part of the patch to make it cleanly apply to
5.2 and 5.3, see remark in the patch itself.
@d-goldin
Copy link
Contributor Author

d-goldin commented Oct 5, 2019

@Mic92: A slightly modified version of the patch has been accepted into the kbuild maintainer's fork. It was modified to include an update to the reproducible builds doc (and a typo fix).

See https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/commit/?h=fixes&id=2cc99c9cdc8fde5e92e34f9655829449cebd3e00

I pulled the updated patch into this PR but commented out the documentation hunk.

@thoughtpolice
Copy link
Member

Fantastic! Let's go ahead and merge this. I think moving to master directly is completely fine; the borg rebuild numbers are doubled for 2x kernel versions, but in general changes like this go to master anyway.

@thoughtpolice
Copy link
Member

@GrahamcOfBorg build linux_5_2 linux_5_3

@globin globin merged commit fba9144 into NixOS:master Oct 8, 2019
@d-goldin d-goldin deleted the fix_linux_5_determinism branch October 8, 2019 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants