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

Multipath: write a test and lightly refactor[CU-1gbcn4c] #1

Merged
merged 5 commits into from
Oct 7, 2021
Merged

Multipath: write a test and lightly refactor[CU-1gbcn4c] #1

merged 5 commits into from
Oct 7, 2021

Conversation

grahamc
Copy link

@grahamc grahamc commented Oct 5, 2021

Motivation for this change
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

grahamc added 4 commits October 5, 2021 15:05
@grahamc
boot.initrd.extraFiles: allow users to add arbitrary files to the initrd
22c3460
@grahamc
iscsi root initiator: allow specifying arbitrary iscsi commands too
9d2cb59
@grahamc
multipath: don't add secrets
e8de489 
I don't think this is appropriate for a module in NixOS.

1. NixOS secrets have somewhat undefined semantics around what
   exactly the initrd contains
2. This is probably assuming that the disk you're using multipath
   for is / or /nix or some other critical-for-boot mount point,
   but that isn't necessarily true.
3. The security properties of initrd secrets are perhaps not well
   defined, and mistaken configuration of /boot could reveal
   important secrets. Indeed, some bootloaders actually require
   placing the "secrets" in the store:

    Failed assertions:
    - boot.loader.initrd.secrets values must be unquoted paths when
    using a bootloader that doesn't natively support initrd
    secrets, e.g.:

      boot.initrd.secrets = {
        "/etc/secret" = /path/to/secret;
      };

    Note that this will result in all secrets being stored
    world-readable in the Nix store!

Instead of actually writing this configuration in this module, it
would probably be better to write documentation about this module
and suggest using boot.initrd.secrets in the situations it is
necessary.
@grahamc
Partially revert 111d406: don't need a -1 flag for multipath-tools
1718961
@flox-clickup-automation flox-clickup-automation bot changed the title Multipath: write a test and lightly refactor Multipath: write a test and lightly refactor[CU-1gbcn4c] Oct 5, 2021
cole-h
cole-h approved these changes Oct 5, 2021
View reviewed changes
Copy link

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

LGTM.

limeytexan
limeytexan reviewed Oct 5, 2021
View reviewed changes
Copy link

@limeytexan limeytexan left a comment

Choose a reason for hiding this comment

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

A few queries, but otherwise looks good! Thanks!

nixos/tests/iscsi-multipath-root.nix Outdated Show resolved Hide resolved
nixos/tests/iscsi-multipath-root.nix Outdated Show resolved Hide resolved
nixos/tests/iscsi-multipath-root.nix Outdated

# Expecting this to fail since we should already know about 192.168.1.3
initiatorAuto.fail("iscsiadm -m discovery -o update -t sendtargets -p 192.168.1.3 --login")
# Expecting this to fail since we should already know about 192.168.2.3
Copy link

Choose a reason for hiding this comment

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

Do you mean fail or succeed in the comment?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, right, good catch. In this case we're expecting it to succeed. Thanks.

nixos/modules/services/networking/multipath.nix
@@ -563,12 +563,9 @@ in {
# with the `-1` argument which disables systemd calls. Invoke `multipath`
# to display the multipath mappings in the output of `journalctl -b`.
boot.initrd.kernelModules = [ "dm-multipath" "dm-service-time" ];
boot.initrd.secrets = {
"/etc/multipath/wwids" = "/etc/multipath/wwids";
Copy link

Choose a reason for hiding this comment

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

I'm concerned this may be necessary when mounting /nix over multipath? I definitely recall needing it at some point - would need to test this on our baremetal installations to know for sure.

Copy link
Author

Choose a reason for hiding this comment

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

It might be, but see my comment here: NixOS#139833 (comment)

Copy link

Choose a reason for hiding this comment

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

Thanks for the pointer! I've responded to the comment, and I think you're right in highlighting that we don't want to update boot.initrd.secrets in all cases. I'm hoping you'll like the idea of a stage1Enable (or similar) variable to control that behaviour. 👍

limeytexan
limeytexan approved these changes Oct 7, 2021
View reviewed changes
Copy link

@limeytexan limeytexan left a comment

Choose a reason for hiding this comment

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

Did a full hands-on invocation of both the test and the changes on my baremetal servers. LGTM.

@limeytexan limeytexan merged this pull request into flox:multipath Oct 7, 2021
limeytexan pushed a commit that referenced this pull request May 21, 2022
@yqrashawn
goku 0.5.1 -> 0.5.2 (#1)
65c8b91
floxbot pushed a commit that referenced this pull request Jul 3, 2023
@amarshall
hashcat: add CUDA support (NixOS#240735)
54769c6 
Previously, hashcat was unable to use CUDA at runtime, and would warn:

> Failed to initialize the NVIDIA main driver CUDA runtime library.
> Failed to initialize NVIDIA RTC library.
> * Device #1: CUDA SDK Toolkit not installed or incorrectly installed.
>              CUDA SDK Toolkit required for proper device support and utilization.
>              Falling back to OpenCL runtime.

This remedies that, at least on NixOS.
floxbot pushed a commit that referenced this pull request Jul 25, 2023
@martinetd
libbpf: 1.2.0 -> 1.2.2
da60cdb 
1.2.1: Bug fix release:

Single bug fix (#1) that fixes regression in `perf` tool caused by libbpf
resetting its custom catch-all `SEC()` handler on explicit
`bpf_program__set_type()` call.

Given setting custom `SEC()` handlers is rarely used and pretty
esoteric feature of libbpf, most users should not be affected.

1.2.2: One more fix:

 - Fix (#2) possible double-free in USDT-related libbpf code, which
happens when libbpf runs out of space in `__bpf_usdt_specs` map due
to having too many unique USDT specs. Running out of space can be
mitigated by bumping up `BPF_USDT_MAX_SPEC_CNT` define before including
`bpf/usdt.bpf.h` header in BPF-side code.
This will prevent the double-free as a side effect (and will make it
possible to successfully attach all requested USDTs), which is a
recommended work-around for libbpf versions prior to v1.2.2.

Link: libbpf/libbpf@e4d3827 #1
Link: libbpf/libbpf@f117080 #2
floxbot pushed a commit that referenced this pull request Aug 7, 2023
@trofi
rtorrent: 0.9.8+date=2021-08-07 -> 0.9.8+date=2022-06-20
0ca93ef 
Pull in _FORTIFY_SOURCE=3 stack smashing fix. Without the change on
current `master` `rtorrent` crashes at start as:

*** buffer overflow detected ***: terminated
                                                                                        __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
44      pthread_kill.c: No such file or directory.
(gdb) bt
    #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
    #1  0x00007ffff7880af3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
    #2  0x00007ffff7831c86 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
    #3  0x00007ffff781b8ba in __GI_abort () at abort.c:79
    #4  0x00007ffff781c5f5 in __libc_message (fmt=fmt@entry=0x7ffff7992540 "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:150
    NixOS#5  0x00007ffff7910679 in __GI___fortify_fail (msg=msg@entry=0x7ffff79924e6 "buffer overflow detected") at fortify_fail.c:24
    NixOS#6  0x00007ffff790eea4 in __GI___chk_fail () at chk_fail.c:28
    NixOS#7  0x00007ffff790ea85 in ___snprintf_chk (s=<optimized out>, maxlen=<optimized out>, flag=<optimized out>, slen=<optimized out>, format=<optimized out>) at snprintf_chk.c:29
    NixOS#8  0x0000000000472acf in utils::Lockfile::try_lock() ()
    NixOS#9  0x000000000044b524 in core::DownloadStore::enable(bool) ()
    NixOS#10 0x00000000004b1f7b in Control::initialize() ()
    NixOS#11 0x000000000043000b in main ()
floxbot pushed a commit that referenced this pull request Mar 4, 2024
@trofi
unnethack: disable fortify3 to avoid startup crash
a956789 
Without the change `unnethack` startup crashes as:

    (gdb) bt
    #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
    #1  0x00007f734250c0e3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
    #2  0x00007f73424bce06 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
    #3  0x00007f73424a58f5 in __GI_abort () at abort.c:79
    #4  0x00007f73424a67a1 in __libc_message (fmt=fmt@entry=0x7f734261e2f8 "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:150
    NixOS#5  0x00007f734259b1d9 in __GI___fortify_fail (msg=msg@entry=0x7f734261e2df "buffer overflow detected") at fortify_fail.c:24
    NixOS#6  0x00007f734259ab94 in __GI___chk_fail () at chk_fail.c:28
    NixOS#7  0x00000000005b2ac5 in strcpy (__src=0x7ffe68838b00 "Shall I pick a character's race, role, gender and alignment for you? [YNTQ] (y)",
        __dest=0x7ffe68838990 "\001") at /nix/store/B0S2LKF593R3585038WS4JD3LYLF2WDX-glibc-2.38-44-dev/include/bits/string_fortified.h:79
    NixOS#8  curses_break_str (str=str@entry=0x7ffe68838b00 "Shall I pick a character's race, role, gender and alignment for you? [YNTQ] (y)", width=width@entry=163,
        line_num=line_num@entry=1) at ../win/curses/cursmisc.c:275
    NixOS#9  0x00000000005b3f51 in curses_character_input_dialog (prompt=prompt@entry=0x7ffe68838cf0 "Shall I pick a character's race, role, gender and alignment for you?",
        choices=choices@entry=0x7ffe68838d70 "YNTQ", def=def@entry=121) at ../win/curses/cursdial.c:211
    NixOS#10 0x00000000005b9ca0 in curses_choose_character () at ../win/curses/cursinit.c:556
    NixOS#11 0x0000000000404eb1 in main (argc=<optimized out>, argv=<optimized out>) at ./../sys/unix/unixmain.c:309

which corresponds to `gcc` warning:

    ../win/curses/cursmisc.c: In function 'curses_break_str':
    ../win/curses/cursmisc.c:275:5: warning: '__builtin___strcpy_chk' writing one too many bytes into a region of a size that depends on 'strlen' [-Wstringop-overflow=]
      275 |     strcpy(substr, str);
          |     ^

I did not find a single small upstream change that fixes it. Let's
disable `fortify3` until next release.

Closes: NixOS#292113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@limeytexan limeytexan limeytexan approved these changes

@cole-h cole-h cole-h approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grahamc @cole-h @limeytexan