common: explain why mlock() failed instead of a bare abort - #9438
common: explain why mlock() failed instead of a bare abort#9438mattraydub wants to merge 1 commit into
Conversation
Andezion
left a comment
There was a problem hiding this comment.
This message only reaches stderr. Operators running lightningd under systemd typically only watch the cln log file, not raw stderr. Was routing this through status_failed/the log considered, or is matching the existing crashdump() convention intentional?
|
|
||
| void mlock_tal_memory(const tal_t *ptr) | ||
| { | ||
| if (sodium_mlock((void *)ptr, tal_bytelen(ptr)) != 0) |
There was a problem hiding this comment.
daemon_setup() already calls err_set_progname(argv0) (via common_setup) before mlock_tal_memory() can run. Did you consider using ccan warn()/warnx() instead of a hand-rolled fprintf + strerror, so the message gets the daemon-name prefix for free and matches the warnx() call in crashdump() that immediately follows it on the same failure path?
There was a problem hiding this comment.
Good catch — no, I'd missed that common_setup() calls err_set_progname(argv0)
(common/setup.c:43) before any of these call sites. Switched to warnx().
One wrinkle in case it comes up: warn() appends : strerror(errno) to the end of
the whole string, which would leave the errno dangling after the Docker/Podman line,
so I've kept warnx() with strerror(errno) inline on the first line where it
belongs. Dropping fprintf also lets the added #include <stdio.h> go again;
<string.h> stays for strerror.
Verified end-to-end on FreeBSD 15.1 / clang 19, in a jail with allow.mlock=0
(which is the case that prompted the patch):
lightning_hsmd: FATAL: could not lock 64 bytes of sensitive memory into RAM: Operation not permitted
Memory locking is required to keep secrets out of swap.
If you are running in a container or jail, the privilege must be granted:
FreeBSD jail: set allow.mlock=1 for the jail
Linux: raise RLIMIT_MEMLOCK (ulimit -l), or grant the CAP_IPC_LOCK capability
Docker/Podman: --ulimit memlock=-1:-1 or --cap-add=IPC_LOCK
lightning_hsmd: FATAL SIGNAL 6 (version 1f86c30-modded)
Which I think makes your point better than I could: with warnx() the diagnostic and
the crashdump() line beneath it now carry the same lightning_hsmd: prefix and read
as one block. Full tree builds clean, no new warnings on common/utils.c (also checked
with -Werror -Wformat=2 added).
77fc67b to
1f86c30
Compare
mlock_tal_memory() aborts when sodium_mlock() fails, with no output at all. The operator sees only "lightning_hsmd: FATAL SIGNAL 6" and has no indication of the cause. This is easy to hit. FreeBSD jails deny mlock(2) unless the jail has allow.mlock set, and containerized Linux deployments hit the same wall against RLIMIT_MEMLOCK. In both cases the failure is indistinguishable from a crash. Keep the abort -- falling back to unlocked memory would silently leave the secret in swappable pages -- but say what failed and how to grant the privilege. Changelog-Fixed: Report the cause when locking secret memory fails, instead of aborting with no diagnostic.
Intentional, though I should have made that explicit in the commit message.
Worth noting the log isn't entirely silent today either: the Happy to revisit if you'd rather have a status-aware variant for the hsmd path |
1f86c30 to
844fb87
Compare
mlock_tal_memory()aborts whensodium_mlock()fails, with no output at all:There is no fallback path and no message, so the operator sees only:
How I found it
A node running fine on 25.09 died on every start after upgrading past 25.12.
Nothing in the logs pointed anywhere useful —
lightningd: HSM sent unknown message typeis emitted, but that is a red herring, justlightningdmisreading an already-dead subdaemon. It took a debug build to get the real
abort site:
The cause was that FreeBSD jails deny
mlock(2)unless the jail hasallow.mlockset. Granting it fixes the node with no other change. Because25.09 and earlier did not lock the secret, the missing permission stays
invisible until the first upgrade past 25.12 — which makes it look like a
release regression rather than a configuration issue.
Containerized Linux deployments hit the same wall against
RLIMIT_MEMLOCKand fail exactly as opaquely.
Full write-up, including the confirmation that the stock package starts
normally once the privilege is granted:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=297253
What this changes
Only the diagnostic. The
abort()stays deliberately — falling back tounlocked memory would silently leave
hsm_secretin swappable pages, andthat is a security policy change that shouldn't ride along on a patch about
error messages. If you'd rather have an opt-out flag for environments that
can't grant the privilege, I'm happy to follow up separately.
New output on failure:
Checks
tools/check-includes.shpasses (stdio.h/string.hadded in thealphabetical position it expects)
-Wformat=2 -Werror(%zuagainsttal_bytelen()'ssize_t)