`store_loadavg_fixed_point()` guarded both of its `<<
LOADAVG_PRECISION_BITS` shifts against overflow, but neither guard was
effective:
- The integer part was rejected with `i >= (~0UL <<
LOADAVG_PRECISION_BITS)`. The shift direction is inverted: that bound is
`0xFFFFFFFFFFFFF800`, so it only rejects the topmost 2048 values of the
range, while the smallest value that actually overflows `i << 11` is
`2^53`. Everything from `2^53` up shifted out of the word and was
accepted.
- The fractional part was shifted first and range-checked afterwards, so
a large `f` wrapped to a small value and then passed the `f >=
LOADAVG_FIXED_POINT_1_0` test.
Concretely, on 64 bit:
```
parse_loadavg_fixed_point("9007199254740992.00", &fp) -> 0, fp == 0 (load reported as 0.00)
parse_loadavg_fixed_point("1.9007199254740992", &fp) -> 0, fp == 1.00
```
Both should be `-ERANGE`. The existing
`ASSERT_ERROR(..."18446744073709551615.5"..., ERANGE)` passes only
because `~0UL` happens to fall inside the narrow band the broken guard
does catch.
Validate both parts before shifting instead. Rejecting `f >= 100` is
exactly equivalent to the old post-shift comparison for every input that
previously succeeded, since `DIV_ROUND_UP(99 << 11, 100) == 2028` and
`DIV_ROUND_UP(100 << 11, 100) == 2048`, so there is no behaviour change
for valid input; only the values that used to wrap are now rejected.
Note this is a latent bug rather than a live one: the only caller of
`parse_loadavg_fixed_point()` is `read_resource_pressure()`, which
parses kernel-generated `/proc/pressure/*` and cgroup `memory.pressure`
files where values are capped at `100.00`, and every direct
`store_loadavg_fixed_point()` caller passes permyriad-derived values
already clamped to 0...10000. Fixing the guards keeps the helper correct
for future callers.
Covered by new cases in `test-parse-util`, including the largest integer
part that must still be accepted (`2^53-1`).
The conversion to the OPTION macros in c3f4c1b8d6 ("journalctl:
convert to OPTION macros") replaced strv_copy_unless_empty() with
strv_copy().
This is problematic because the positional arguments are now an empty
strv rather than NULL when none are given, but `--{list,dump}-catalog`
rely on that.
Instead let's hand NULL back to them when there is nothing on the
command line.
If dbus dies, it is then immediately reactivated by dbus.socket as soon
as a client reconnects. In that case pid1's own connection to the bus
drops and is reestablished by manager_recheck_dbus() (which comes from
the unit_notify() when dbus.services goes back to being running).
As part of bringing the bus back up, bus_setup_api() issues
GetNameOwner() for every name we watch on behalf of a BusName= unit.
For a Type=dbus service this is a problem. Consider this scenario:
1. spearmint.service is Type=dbus, owns com.example.Spearmint, and is
running.
2. The broker crashes and is re-activated through dbus.socket.
3. spearmint's client connection dropped too, so it is now reconnecting
to the new broker to grab its name again.
4. pid1 racily reconnects first given it is triggered the moment
dbus.service reaches SERVICE_RUNNING, whereas spearmint is an
ordinary client that must first notice its own disconnection and thus
when we do GetNameOwner() com.example.Spearmint is reported as having
no owner.
5. service_bus_name_owner_change() sets bus_name_good = false, and since
service_good() returns false for a Type=dbus service without its
name, service_enter_running() stops the (perfectly healthy, other
than dbus kicking the bucket temporarily) service.
In other words, a broker restart can tear down running Type=dbus
services that did nothing wrong, really.
So, to fix this, we can use the fact that a NameOwnerChanged signal is
definitive. That is, the name's owner changed while we were connected,
so a service that crashes or drops its name can still be stopped
immediately. By contrast a GetNameOwner() reply is less authoritative.
There is, importantly, no change to how losing the name on a live
connection works, we only give grace on reconnect.
For a Type=dbus service we sometimes want to keep treating it as running
while its bus name has briefly gone away (for example, due to a broker
restart).
SERVICE_RUNNING_REVALIDATING is active and keeps its main process like
SERVICE_RUNNING, but it's deliberately not part of the runtime timer
set, so RuntimeMaxSec= is paused while we sit in it.
Add a new integration test to verify the signal handler validation for
Type=notify-reload services, as introduced in the previous commit.
The test adds a shell harness and four service files to cover all scenarios:
1. `notify-reload-no-handler.service`: Verifies that the service fails to
start with Result=protocol when the handler is missing at the first
READY=1.
2. `notify-reload-sigstop.service`: Verifies that SIGSTOP is exempt from
handler validation because it cannot be caught or blocked.
3. `notify-reload-toggle-handler.service`: Verifies that removing the
handler at runtime results in a warning on reload, but the signal is still
sent, causing the service to terminate from unhandled SIGHUP.
4. `notify-reload-well-behaved.service`: Verifies the happy path where a
service with a handler starts and reloads successfully.
Use bounded FIFO operations and journal cursors so failures are prompt and log
assertions cannot race a relative time window.
We currently blindly send the configured reload signal (e.g. SIGHUP or
SIGUSR1) to the main service PID for Type=notify-reload units, even if
the service hasn't installed a userspace handler. This can lead to
invoking unintended default behavior (typically process termination) in
daemons which later deprecate and remove their reload handler.
This is a real problem we have seen in production on multiple occasions.
In one particularly egregious instance, a production distributed storage
service had a large percentage of its nodes all terminate at once when
sent a reload signal. In this case a signal handler had been removed, but
another place still sending the signal was missed.
To mitigate this, introduce signal handler validation for
Type=notify-reload services. We check both SigCgt (traditional handlers
via sigaction) and SigBlk (blocked signals for signalfd) to detect valid
handler configurations:
1. On first READY=1: When the service first sends READY=1 during initial
startup, we check for the handler. If it's missing, the service
startup is aborted with Result=protocol.
2. On reload: Immediately before sending the reload signal, we check
again. If the handler is missing, we log a warning but still send
the signal.
The rationale is that a missing handler at first startup represents a
definite misconfiguration that should be caught early. A missing handler
at reload time is warned about, but we defer to the operator's judgement
since the service already started successfully.
These checks are a best-effort, pragmatic safety net. They are naturally
racy, but in practice they prevent the common class of bugs from static
misconfiguration or software updates.
Add new pidref_has_sigcgt() and pidref_has_sigblk() helpers. These read
the SigCgt and SigBlk bitmasks from /proc/<pid>/status to detect either
traditional signal handlers installed via sigaction(2), or blocked signals
that are typically handled via signalfd(2), such as through
sd_event_add_signal().
Both mechanisms represent valid ways to handle a signal without invoking
the default action. Share the parsing and pidref verification between the
two public helpers.
sd_bus_message carries an array of header field offsets and a counter,
but their only reader was part of the D-Bus v2/GVariant sealing path
removed in 0dd4876815 ("sd-bus: drop D-Bus version 2 format support").
There are three remaining callers of message_extend_fields(), for
SD_BUS_MESSAGE_HEADER_DESTINATION, _PATH, and _INTERFACE, and all of
them pass false for add_offset. The receive path does not populate the
array either, so nothing reads or writes it any more.
Let's drop the two fields and the now unused add_offset argument and
branch.
This reduces sizeof(sd_bus_message) from 792...
$ gdb -batch build-baseline/test-bus-benchmark \
-ex 'ptype /o struct sd_bus_message'
...
/* 688 | 8 */ usec_t timeout;
/* 696 | 80 */ size_t header_offsets[10];
/* 776 | 4 */ unsigned int n_header_offsets;
/* XXX 4-byte hole */
/* 784 | 8 */ uint64_t read_counter;
/* total size (bytes): 792 */
to 704 bytes:
$ gdb -batch build-patched/test-bus-benchmark \
-ex 'ptype /o struct sd_bus_message'
...
/* 688 | 8 */ usec_t timeout;
/* 696 | 8 */ uint64_t read_counter;
/* total size (bytes): 704 */
On Fedora 43 aarch64 with glibc 2.42, the usable allocation for a
method-call message falls from 808 to 728 bytes, a reduction of 9.9%.
In my tests, retaining 400,000 method-call messages reduces in median
peak RSS from 363M to 332M.
Using `test-bus-benchmark chart direct 500ms` across message sizes from
1 byte to 2 MiB one can also see things are around 1% faster, which is
another nice incidental boost.
bus_message_print_all_properties() builds a PROP= string for every
property in the reply so that -p PROP=value filters can be matched
against it, but most queries never need this.
Take the normal `systemctl show` or `systemctl show UNIT` case. In that
case there is no filter. Even with `-p PROP` there is no value filter
since there is no value.
Avoid constructing the string entirely by comparing property names
directly against filter entries.
In my tests with a `systemctl show` over 160 units this brings the
instructions retired from 992.6M down to 960.5M, a reduction of 3.2%.
The same goes for property filters with units. When running:
systemctl show -p UnitFileState -p ActiveState UNIT
...the instructions retired drops from 9.52M to 9.22M, a reduction of
3.2%. The output in each case is unchanged.
bus_message_print_all_properties() peeks the variant type, but then the
print callback and the default bus_print_property() each peek the value
type again, so there can be up to two redundant calls per property. Peek
it once up front and pass it through.
With this, in my tests `systemctl show` over 160 units decreases in
instructions retired from 1167.6M to 1155.7M, so about 1%.
bus_message_print_all_properties() peeks the variant type, but then the
print callback and the default bus_print_property() each peek the value
type again, so there can be up to two redundant calls per property. Peek
it once up front and pass it through.
With this, in my tests `systemctl show` over 160 units decreases in
instructions retired from 1167.6M to 1155.7M, so about 1%.
bus_message_print_all_properties() inserts every name it walks into the
found-properties set, but the set is only used to report missing requested
properties at debug level.
Request the set from systemctl only when properties were specified and debug
logging is enabled, avoiding the unnecessary work in normal operation.
In my tests with `systemctl show` over 160 units this brings the
instructions retired from 1167.8M down to 992.6M, a reduction of 15.0%.
The output is unchanged.
The data and field hash table chains have the same problem the previous
commit fixed for entry array chains. New data and field objects are
linked at the tail of their hash bucket by patching the previous tail
object's next_hash_offset in place, so after a crash a persisted
predecessor (or the bucket head) can point at an object whose body never
reached disk.
journal_file_find_data_object_with_hash() and
journal_file_find_field_object_with_hash() walk those chains while
resolving matches, and on -EADDRNOTAVAIL/-EBADMSG from
journal_file_move_to_object() they simply return the error directly.
That propagates up to real_journal_next(), which discards the whole file
from the query.
Give those two lookups the same tolerance: on a read-only file, treat an
unreadable chain node as the end of the bucket chain.
generic_array_get() which is used for the unfiltered iteration path in
the previous commit treats a chain pointer that resolves past the end of
the file as the end of the chain. In that case, moving to the missing
array object returns -EADDRNOTAVAIL (or -EBADMSG), and it either stops
(going downwards) or steps back to the previous array (going upwards).
However, generic_array_bisect(), which is used for filtered or seeking
reads does not. On -EADDRNOTAVAIL/-EBADMSG from
journal_file_move_to_object(), it instead returns the error directly to
the caller, which propagates out through
sd_journal_next()/sd_journal_previous() and aborts the query.
The per-data entry array chain has the same issue as the global one,
since n_entries and entry_array_offset are (re)written in place as
entries are linked, and thus after a crash they can reference more
arrays than actually reached the disk. That is to say in practical
terms, a journal recovered for reading by the previous commit could
nevertheless still drop matching entries from `journalctl FIELD=value`,
and a seqnum or time seek into the lost region could fail outright.
Let's give generic_array_bisect() the same tolerance generic_array_get()
already has. That is, when moving to an entry array object fails, treat
the chain as ending at the previous array. This means that the result
matches what generic_array_get() would yield for the same file.
In Meta production we have been considering using journald more widely
for some time. One of the blockers to doing that which I have noticed is
that often journald seems to have vastly less data after lockups/power
failures compared to plain files, which is not great when debugging
outages.
On small write rates this tends to be hard to reproduce, but when
writing thousands of messages a second, an unclean shutdown can result
in the end result being an active journal file with a header that
records an arena larger than the data that actually reached disk. What
happens is then that journalctl then discards the entire file(!),
completely ignoring that there is a huge amount of data which is
actually perfectly readable.
The reason for that is that the journal header is updated on every
append, while the file size and newly written arena contents are only
made durable on the filesystem's own schedule. After a crash, the header
can therefore describe writes which were logically completed by journald
but whose backing data or file metadata never reached disk.
Take the following example of how this can happen at high log rates:
1. journald appends objects into an mmap()ed arena, periodically growing
the file with fallocate() in FILE_SIZE_INCREASE (8M) steps and
advancing the header's arena_size tail pointers as it goes along.
2. The header is dirtied on every append, and its arena_size is advanced
at each fallocate(). It is, from the kernel's perspective, an
ordinary data page and is only made durable by the kernel's periodic
page cache writeback on its own schedule. The file's length, by
contrast, is metadata, made durable only when the filesystem commits
a transaction (or on an fsync(), which journald does not issue
between sync intervals).
3. journald marks journals NOCOW, so the header's data block is
overwritten in place and is decoupled from the size metadata. Nothing
orders the two with respect to each other. Writeback therefore can
routinely persist a header whose arena_size has run ahead of the file
length recorded on disk.
4. Power is lost. On the next boot the persisted header reflects an
arena_size and tail pointers which have been advanced for appends.
However their payload and the file metadata were never committed, so
header_size + arena_size now points well past the end of the file as
it exists on disk.
5. journal_file_verify_header() then rejects this with -ENODATA:
if (... || header_size + arena_size > (uint64_t) f->last_stat.st_size)
return -ENODATA;
That is correct when opening for writing, because we must not append to
a file whose recorded state we cannot trust, and the caller must rotate
it away. But the same check also runs on read only opens, where it is
actively harmful. In the case of journalctl, the entire file is skipped,
even though the data hash table, the field hash table, and the head of
the array all are present and fully intact, and the great majority of
entries are physically present. In fact, only a very small part of the
most recently written tail is missing, but everything before is
readable. This results in mistakenly rejecting the entire file as
corrupt.
This happens extremely frequently on machines with high write rates
during power cuts or lockups. In testing writing ~7500 msg/s through
journald and then cutting power, I reproduced it in ten out of ten
attempts across different machines.
In each case, the header was left claiming ~296M of arena while only
~192-208M had reached disk. In this case, journalctl reports that it has
recovered 0 of ~335000 messages. Whether a given crash trips the
condition depends on where it falls relative to the header's writeback,
but when it does, the loss today is total. After this patch the vast
majority of messages can be retrieved.
Let's fix this by keeping the rejection for writing, but for read-only
opens, let's just clamp the arena to the real file size and skip the
consistency checks on the now unreliable tail pointers. The reader will
walk the entry array chain from its intact head and stop at the
truncation point by the bounds check that already exists, so there's no
need to do any more than that there.
You might also wonder, why not address this on the write side? That
would be astronomically expensive and require an fsync() after every
fallocate().
In terms of improvements, when reading from a file affected in the way
described above, previously journalctl recovers 0 entries, and now it
can recover all actually intact entries.
The fix for the corrupted state when units become aliased on reload
leaks the now-aliased unit's resources, which become untracked and
essentially lost.
While fixing the state corruption is of course necessary, leaking
processes/etc. is not ideal for a system and service manager, so
instead attempt to keep track of them by creating stub units
on-the-fly.
This way resources are not leaked, there are clear indications of
where they moved, and all state can be tracked as expected.
When systemd-cryptenroll --tpm2-device=auto is called on a system where
a tpm2-pcr-public-key.pem exists it automatically creates tokens with a
signed PCR policy. Unlocking such a token via --unlock-tpm2-device=auto
requires a tpm2-pcr-signature.json file, which is not present.
This creates a race with systemd-tpm2-setup.service at boot: if the
service completes before the test, the key exists and the subsequent
--unlock-tpm2-device=auto calls fail, which I believe is the cause of
the test flakiness.
This also seems to mesh with the fact that this only flakes on Debian
CI, since that's built with ukify which installs a public key.
Let's hopefully fix this by passing --tpm2-public-key= to all
--tpm2-device= enrollment calls that aren't explicitly intended to test
signed PCR policy behaviour.
During daemon-reload (or daemon-reexec), when a unit becomes an alias to
another unit, deserialising the alias's stale serialised state can
corrupt the canonical unit's live runtime state.
Consider this scenario:
1. Before reload:
- a.service is running
- b.service was stopped earlier and is dead
- Both exist as independent units
2. User creates an alias to migrate from b -> a:
- `ln -s /run/systemd/system/a.service /etc/systemd/system/b.service`
3. daemon-reload triggers serialisation. State file contains both units:
- a.service -> state=running, cgroup=/system.slice/a.service, PID=1234,
...
- b.service -> state=dead, cgroup=(empty), no PIDs, ...
4. During deserialisation:
- Processes a.service: loads Unit A, deserialises -> state=RUNNING
- Processes b.service: manager_load_unit() detects symlink, returns Unit
A
- unit_deserialize_state(Unit A, ...) overwrites with b's dead state
5. The result is that:
- Unit A incorrectly shows state=dead despite PID 1234 still running
- If a.service has Upholds= dependents, catch-up logic sees a.service
should be running but is dead
- systemd starts a.service again -> PID 5678
- Two instances run: PID 1234 (left-over) and PID 5678 (new)
This bug is deterministic when serialisation orders a.service before
b.service.
The root cause is that manager_deserialize_one_unit() calls
manager_load_unit(name, &u) which resolves aliases via
unit_follow_merge(), returning the canonical Unit object. However, the
code doesn't distinguish between two cases when u->id differs from the
requested name from the state file. In the corruption case, we're
deserialising an alias entry and unit_deserialize_state() blindly
overwrites the canonical unit's fields with stale data from the old,
independent unit. The serialised b.service then overwrites Unit A's
correct live state.
This commit first scans the serialised unit names, then adds a check
after manager_load_unit():
if (!streq(u->id, name) && set_contains(serialized_units, u->id))
...
This detects when the loaded unit's canonical ID (u->id) differs from
the serialised name, indicating the name is now an alias for a different
unit and the canonical unit also has its own serialised state entry.
If the canonical unit does not have its own serialised state entry, we
keep the state entry. That handles cases where the old name is really
just a rename, and thus the old name is the only serialised state for
the unit. In that case there is no bug, because there is no separate
canonical state entry for the stale alias entry to overwrite.
Skipping is safe because:
1. The canonical unit's own state entry will be correctly deserialised
regardless of order. This fix only prevents other stale alias entries
from corrupting it.
2. unit_merge() has already transferred the necessary data. When
b.service became an alias during unit loading, unit_merge() already
migrated dependencies and references to the canonical unit.
3. After merging, the alias doesn't have its own runtime state. The
serialised data represents b.service when it was independent, which is
now obsolete once the canonical unit also has its own serialised entry.
4. All fields are stale. unit_deserialize_state() would overwrite state,
timestamps, cgroup paths, pids, etc. There's no scenario where we want
this data applied on top of the canonical unit's own serialised state.
This fix also correctly handles unit precedence. For example, imagine
this scenario:
1. `b.service` is a valid, running unit defined in `/run`.
2. The sysadmin creates `ln -s .../a.service /etc/.../b.service`.
3. On reload, the new symlink in `/etc` overrides the unit in `/run`.
The new perspective from the manager side is that `b.service` is now an
alias for `a.service`.
In this case, systemd correctly abandons the old b.service unit, because
that's the intended general semantics of unit file precedence. We also
do that in other cases, like when a unit file in /etc/systemd/system/
masks a vendor-supplied unit file in /lib/systemd/system/, or when an
admin uses systemctl mask to explicitly disable a unit.
In all these scenarios, the configuration with the highest precedence
(in /etc/) is treated as the new source of truth. The old unit's
definition is discarded, and its running processes are (correctly)
abandoned. In that respect we are not doing anything new here.
Some may ask why we shouldn't just ignore the symlink if we think this
case will come up. I think there are multiple very strong reasons not to
do so:
1. It violates unit precedence. The unit design is built on a strict
precedence list. When an admin puts any file in /etc, they are
intentionally overriding everything else. If manager_load_unit were to
"ignore" this file based on runtime state, it would break this
fundamental precedent.
2. It makes daemon-reload stateful. daemon-reload is supposed to be a
simple, stateless operation, basically to read the files on disk and
apply the new configuration. But doing this would make daemon-reload
stateful, because we'd have to read the files on disk, but
cross-reference the current runtime state, and... maybe ignore some
files. This is complex and unpredictable.
3. It also completely ignores the user intent. The admin clearly has
tried to replace the old service with an alias. Ignoring their
instruction is the opposite of what they want.
Fixes: https://github.com/systemd/systemd/issues/38817
Fixes: https://github.com/systemd/systemd/issues/37482
During daemon-reload (or daemon-reexec), when a unit becomes an alias to
another unit, deserialising the alias's stale serialised state can
corrupt the canonical unit's live runtime state.
Consider this scenario:
1. Before reload:
- a.service is running
- b.service was stopped earlier and is dead
- Both exist as independent units
2. User creates an alias to migrate from b -> a:
- `ln -s /run/systemd/system/a.service /etc/systemd/system/b.service`
3. daemon-reload triggers serialisation. State file contains both units:
- a.service -> state=running, cgroup=/system.slice/a.service,
PID=1234, ...
- b.service -> state=dead, cgroup=(empty), no PIDs, ...
4. During deserialisation:
- Processes a.service: loads Unit A, deserialises -> state=RUNNING
- Processes b.service: manager_load_unit() detects symlink, returns
Unit A
- unit_deserialize_state(Unit A, ...) overwrites with b's dead state
5. The result is that:
- Unit A incorrectly shows state=dead despite PID 1234 still running
- If a.service has Upholds= dependents, catch-up logic sees
a.service should be running but is dead
- systemd starts a.service again -> PID 5678
- Two instances run: PID 1234 (left-over) and PID 5678 (new)
This bug is deterministic when serialisation orders a.service before
b.service.
The root cause is that manager_deserialize_one_unit() calls
manager_load_unit(name, &u) which resolves aliases via
unit_follow_merge(), returning the canonical Unit object. However, the
code doesn't distinguish between two cases when u->id differs from the
requested name from the state file. In the corruption case, we're
deserialising an alias entry and unit_deserialize_state() blindly
overwrites the canonical unit's fields with stale data from the old,
independent unit. The serialised b.service then overwrites Unit A's
correct live state.
This commit first scans the serialised unit names, then adds a check
after manager_load_unit():
if (!streq(u->id, name) && set_contains(serialized_units, u->id))
...
This detects when the loaded unit's canonical ID (u->id) differs from
the serialised name, indicating the name is now an alias for a different
unit and the canonical unit also has its own serialised state entry.
If the canonical unit does not have its own serialised state entry, we
keep the state entry. That handles cases where the old name is really
just a rename, and thus the old name is the only serialised state for
the unit. In that case there is no bug, because there is no separate
canonical state entry for the stale alias entry to overwrite.
Skipping is safe because:
1. The canonical unit's own state entry will be correctly deserialised
regardless of order. This fix only prevents other stale alias entries
from corrupting it.
2. unit_merge() has already transferred the necessary data. When
b.service became an alias during unit loading, unit_merge() already
migrated dependencies and references to the canonical unit.
3. After merging, the alias doesn't have its own runtime state. The
serialised data represents b.service when it was independent, which
is now obsolete once the canonical unit also has its own serialised
entry.
4. All fields are stale. unit_deserialize_state() would overwrite state,
timestamps, cgroup paths, pids, etc. There's no scenario where we
want this data applied on top of the canonical unit's own serialised
state.
This fix also correctly handles unit precedence. For example, imagine
this scenario:
1. `b.service` is a valid, running unit defined in `/run`.
2. The sysadmin creates `ln -s .../a.service /etc/.../b.service`.
3. On reload, the new symlink in `/etc` overrides the unit in `/run`.
The new perspective from the manager side is that `b.service` is now an
alias for `a.service`.
In this case, systemd correctly abandons the old b.service unit, because
that's the intended general semantics of unit file precedence. We also
do that in other cases, like when a unit file in /etc/systemd/system/
masks a vendor-supplied unit file in /lib/systemd/system/, or when an
admin uses systemctl mask to explicitly disable a unit.
In all these scenarios, the configuration with the highest precedence
(in /etc/) is treated as the new source of truth. The old unit's
definition is discarded, and its running processes are (correctly)
abandoned. In that respect we are not doing anything new here.
Some may ask why we shouldn't just ignore the symlink if we think this
case will come up. I think there are multiple very strong reasons not to
do so:
1. It violates unit precedence. The unit design is built on a strict
precedence list. When an admin puts any file in /etc, they are
intentionally overriding everything else. If manager_load_unit were
to "ignore" this file based on runtime state, it would break this
fundamental precedent.
2. It makes daemon-reload stateful. daemon-reload is supposed to be a
simple, stateless operation, basically to read the files on disk and
apply the new configuration. But doing this would make daemon-reload
stateful, because we'd have to read the files on disk, but
cross-reference the current runtime state, and... maybe ignore some
files. This is complex and unpredictable.
3. It also completely ignores the user intent. The admin clearly has
tried to replace the old service with an alias. Ignoring their
instruction is the opposite of what they want.
Fixes: https://github.com/systemd/systemd/issues/38817
Fixes: https://github.com/systemd/systemd/issues/37482
I noticed the message for verity partition mismatch was wrong, which
then led me down this rabbit hole...
Aside from the new test for usr verity partitions, this is 49
insertions(+), 127 deletions(-), and reduces the space for bugs to hide
a bit.
The verity consistency checks and verity setup code also have parallel
blocks for root and usr that do basically identical work. Let's
consolidate them and reduce the footprint for bugs or deviance to
manifest.
dissect-image has six(!) different branches with basically the same
code. Let's avoid that and reduce the spaces for bugs or differing
behaviour to subtly creep in.
Let's say a user has two services with ManagedOOMMemoryPressure=kill,
perhaps a web server under system.slice and a batch job under
user.slice. Both exceed their pressure limits. On the previous timer
tick, oomd has already queued the web server's candidate for killing,
but the prekill hook has not yet responded, so the kill is still
pending.
In the code, monitor_memory_pressure_contexts_handler() iterates over
all pressure targets that have exceeded their limits. When it reaches
the web server target and calls oomd_cgroup_kill_mark(), which returns 0
because that cgroup is already queued. The code treats this the same as
a successful new kill: it resets the 15 second delay timer and returns
from the function, exiting the loop.
This loop is handled by SET_FOREACH and the iteration order is
hash-dependent. As such, if the web server target happens coincidentally
to be visited first, oomd never evaluates the batch job target at all.
The effect is twofold:
1. oomd stalls for 15 seconds despite not having initiated any new kill.
That can unnecessarily delay further action to stem increases in
memory pressure. The delay exists to let stale pressure counters
settle after a kill, but no kill has happened here.
2. It non-deterministically skips pressure targets that may have
unqueued candidates, dangerously allowing memory pressure to persist
for longer than it should.
Fix this by skipping cgroups that are already queued so the loop
proceeds to try other pressure targets. We should only delay when a new
kill mark is actually created.
Consider a workload slice with several sibling cgroups. Imagine that one
of those cgroups is removed between the moment oomd enumerates the
directory and the moment it reads memory.oom.group. This is actually
relatively plausible under the high memory pressure conditions where
oomd is most needed.
In this case, the failed read prompts us to `return 0`, which exits the
entire enumeration loop in recursively_get_cgroup_context(). As a
result, all remaining sibling cgroups are silently dropped from the
candidate list for that monitoring cycle.
The effect is that oomd can fail to identify and kill the actual
offending cgroup, allowing memory pressure to persist until a subsequent
cycle where the race doesn't occur.
Fix this by instead proceeding to evaluate further sibling cgroups.
Let's say a user has two services with ManagedOOMMemoryPressure=kill,
one for a web server under system.slice, and one for a batch job under
user.slice. The batch job is causing severe memory pressure, whereas the
web server's cgroup has no processes with significant pgscan activity.
In the code, monitor_memory_pressure_contexts_handler() iterates over
all pressure targets that have exceeded their limits. When
oomd_select_by_pgscan_rate() returns 0 (that is, no candidates) for a
target, we return from the entire SET_FOREACH loop instead of moving to
the next target. Since SET_FOREACH iteration order is hash-dependent, if
the web server target happens to be visited first, oomd will find no
kill candidates for it and exit the loop. The batch job target that is
actually slamming the machine will never even be evaluated, and can
continue to wreak havoc without any intervention.
The effect is that oomd non-deterministically and silently fails to kill
cgroups that it should actually kill, allowing memory pressure to
persist and dangerously build up on the machine.
The fix is simple, keep evaluating remaining targets when one does not
match.
Emitting "oom" doesn't mesh with the org.freedesktop.oom1.Manager
Killed() API contract, which defines "memory-used" and "memory-pressure"
as possible reasons. Consumers that key off reason thus will either lose
policy attribution or may reject the unknown value completely.
Plumb the reason through so it is visible to consumers.
oomd_cgroup_kill_mark() currently returns 0 on all non-error paths. But
the manager only logs that it marked for killing on `if (r > 0)`, which
is thus unreachable.
Changing it to `r >= 0` would also be wrong, because then we would log
on no-op paths.
So let's fix this by making the return value express what actually
happened:
- < 0: failure to queue the kill state
- 0: no new mark was created (already queued or dry-run)
- > 0: a new kill state was queued
oomd_cgroup_kill_mark() allocates a temporary OomdKillState and inserts
it into kill_states via set_ensure_put(). This is keyed by cgroup path.
When the same cgroup is already queued, set_ensure_put() dutifully
returns 0.
The function then returns with
_cleanup_(oomd_kill_state_removep) still armed, which eventually calls
oomd_kill_state_free().
oomd_kill_state_free() removes from kill_states by cgroup-path key, so
because this path already exists, it will remove the existing queued
kill state instead of just dropping the temporary object.
This is wrong and results in mistakenly drops the queued kill state on
duplicates.
This can happen when a cgroup is marked multiple times before the first
queued kill state is consumed. The result is lost kill-state tracking
and incorrect prekill/kill sequencing.
Handle r == 0 explicitly by freeing only the temporary object and
leaving the already queued state intact.
While looking at oomd behaviour in production I noticed that I always
get garbage cgroup paths for the Killed event. Looking more closely, I
noticed that while the signature is (string cgroup, string reason), we
currently erroneously pass the `OomdCGroupContext*` pointer itself as
the first argument to sd_bus_emit_signal(), rather than the ctx->path
string it contains.
The in-memory layout on affected machines in my case is:
struct OomdCGroupContext {
unsigned n_ref;
/* padding */
char *path;
/* ... */
}
...which explains the control characters, since they're garbage from
parsing n_ref, the path pointer, and later fields. At runtime, sd-bus
treats ctx as `const char *` and reads struct bytes as string data,
resulting in garbage being sent.
Pass ctx->path correctly so listeners receive the valid cgroup path.
string_has_ansi_sequence() currently does this to look for ESC input:
t = memchr(s, 0x1B, ...)
So each iteration re-searches from the original start pointer. But if we
find an ESC byte that does *not* start a valid ANSI sequence (like "\x1B
", or an ESC at the end of the string), then ansi_sequence_length()
returns 0, and if that ESC is still in the search window, we will just
spin consuming 100% CPU forever.
Fix this by always advancing past rejected ESC bytes.
get_block_device_harder_fd() currently only traverses one level of
device mapper stacking when looking for the underlying block device.
This causes issues with nested DM setups like dm-crypt on top of
dm-integrity, where we don't traverse enough to get the actual physical
device.
Fix this by iterating through all DM layers until we reach a device with
no underlying device.
Fixes: #40419
device_track_back() only checks a single DM layer when resolving the
originating device. Use
block_device_get_originating(..., /* recursive= */ true) to follow
stacked layers.
Originating device lookups currently use separate entry points and
callers must decide between the single-step helper and manual looping.
Add a recursive flag to block_device_get_originating() and
block_get_originating(), and update callers to pass an explicit value.
Add unit tests for config_parse_multipath_route() covering the new
device-only nexthop syntax (@device). Tests verify basic parsing of
device-only routes, gateway with device routes, gateway-only routes,
interface index handling, etc. Also some checks on new hash/duplicate
semantics.
Also add an integration test to verify device-only multipath routes are
correctly installed in the kernel routing table.
MultiPathRoute= can now specify device-only nexthops without a gateway
address, e.g. MultiPathRoute=@wg0. This enables ECMP configurations over
interfaces that don't use gateway addresses, such as WireGuard tunnels.
The syntax is extended from "address[@device] [weight]" to
"[address]@device [weight]". The address is now optional, but at least
one of gateway or device must be specified. The @ symbol must still be
present for device-only routes, making the syntax unambiguous: @wg0
specifies a device, while a bare IP address specifies a gateway.
Device-only nexthops are only available for IPv4 routes. Device-only
multipath routes for IPv6 are not supported by the kernel's netlink
interface and will be rejected with a warning.
This change is fully backwards compatible. All existing configurations
continue to work unchanged, as they always included a gateway address.
Closes#39699.
Commit 88252ca changed nspawn to always run from a temporary mount
directory (e.g., /tmp/nspawn-root-XXXXXX). This was a good
simplification for mount logic, but it unintentionally broke the
--link-journal feature.
The setup_journal() helper was subsequently passed this ephemeral path
instead of the persistent machine path (from --directory= or --image=).
This caused the host to create broken symlinks pointing to a temporary
directory that would soon be gone.
Fix this by storing the original path and plumbing it through to
setup_journal().
All other mount-related logic in outer_child() continues to use the
temporary `directory` variable.
Fixes: #39472
Commit 88252ca changed nspawn to always run from a temporary mount
directory (e.g., /tmp/nspawn-root-XXXXXX). This was a good
simplification for mount logic, but it unintentionally broke the
--link-journal feature.
The setup_journal() helper was subsequently passed this ephemeral path
instead of the persistent machine path (from --directory= or --image=).
This caused the host to create broken symlinks pointing to a temporary
directory that would soon be gone.
Fix this by storing the original path and plumbing it through to
setup_journal().
All other mount-related logic in outer_child() continues to use the
temporary `directory` variable.
Fixes: #39472
This series fixes two distinct, pretty bad bugs in `ASSERT_SIGNAL`.
These bugs can allow failing tests to pass, and can also cause the test
runner to silently terminate prematurely in a way that looks like
success.
This is not theoretical, see
https://github.com/systemd/systemd/pull/39674#discussion_r2540552699 for
a real case of this happening.
---
Bug 1: Parent process hallucinates it is the child and re-executes the
expression being tested
Previously, assert_signal_internal() returned 0 in two mutually
exclusive states:
1. We are the child process (immediately after fork()).
2. We are the parent process, and the child exited normally (status 0).
The macro failed to distinguish these cases. If a child failed to crash
as expected, the parent received 0, incorrectly interpreted it as it
being the child, and re-executed the test expression inside the parent
process.
This can cause tests to falsely pass. The parent would successfully run
the expression (which wasn't supposed to crash in the parent), succeed,
and call _exit(EXIT_SUCCESS).
The second consequence is silent truncation. When the parent called
_exit(), it terminated the entire test runner immediately. Any
subsequent tests in the same binary were never executed.
---
Bug 2: Conflation of exit codes and signals
The harness returned the raw si_status without checking si_code. This
meant that an exit code was indistinguishable from a signal number. For
example, if a child process failed and called exit(6), the harness
reported it as having been killed by SIGABRT (signal 6).
---
This PR both fixes the bugs and reworks the ASSERT_SIGNAL infrastructure
to ensure this is very unlikely to regress:
- assert_signal_internal now returns an explicit control flow enum
(FORK_CHILD / FORK_PARENT) separate from the status data. This makes it
structurally impossible for the parent to hallucinate that it is the
child.
- The output parameter is only populated with a signal number if si_code
confirms the process was killed by a signal. Normal exits return 0.
assert_signal_internal() returns 0 in two distinct cases:
1. In the child process (immediately after fork returns 0).
2. In the parent process, if the child exited normally (no signal).
ASSERT_SIGNAL fails to distinguish these cases. When a child exited
normally (case 2), the parent process receives 0, incorrectly interprets
it as meaning it is the child, and re-executes the test expression
inside the parent process. Goodness gracious!
This causes two severe test integrity issues:
1. False positives. The parent can run the expression, succeed, and call
_exit(EXIT_SUCCESS), causing the test to pass even though no signal
was raised.
2. Silent truncation. The _exit() call in the parent terminates the test
runner prematurely, preventing subsequent tests in the same file from
running.
Example of the bug in action, from #39674:
ASSERT_SIGNAL(fd_is_writable(closed_fd), SIGABRT)
This test should fail (fd_is_writable does not SIGABRT here), but with
the bug, the parent hallucinated being the child, re-ran the expression
successfully, and exited with success.
Fix this by refactoring assert_signal_internal() to be much more strict
about separating control flow from data.
The signal status is now returned via a strictly typed output parameter,
guaranteeing that determining whether we are the child is never
conflated with whether the child exited cleanly.
ASAN installs signal handlers to catch crashes like SIGSEGV or SIGILL.
When these signals are raised, ASAN traps them, prints an error report,
and then typically terminates the process with a different signal (often
SIGABRT) or a non-zero exit code.
This interferes with ASSERT_SIGNAL when checking for specific crash
signals (for example, checking that a function raises SIGSEGV). In such
a case, the test harness sees the ASAN termination signal rather than
the expected signal, causing the test to fail.
Fix this by resetting the signal handler to SIG_DFL in the child process
immediately before executing the test expression. This ensures the
kernel kills the process directly with the expected signal, bypassing
ASAN's interceptors.
When a child process exits normally (si_code == CLD_EXITED),
siginfo.si_status contains the exit code. When it is killed by a signal
(si_code == CLD_KILLED or CLD_DUMPED), si_status contains the signal
number. However, assert_signal_internal() returns si_status blindly.
This causes exit codes to be misinterpreted as signal numbers.
This allows failing tests to silently pass if their exit code
numerically coincides with the expected signal. For example, a test
expecting SIGABRT (6) would incorrectly pass if the child simply exited
with status 6 instead of being killed by a signal.
Fix this by checking si_code. Only return si_status as a signal number
if the child was actually killed by a signal (CLD_KILLED or CLD_DUMPED).
If the child exited normally (CLD_EXITED), return 0 to indicate that no
signal occurred.
When inheriting file descriptors for stdout/stderr (either from stdin
or when making stderr inherit from stdout), we previously just assumed
they would be writable and dup'd them. This could lead to broken setups
if the inherited FD was actually opened read-only.
Before dup'ing any inherited FDs to stdout/stderr, verify they are
actually writable using the new fd_is_writable() helper. If not, fall
back to /dev/null (or reopen the terminal in the TTY case) with a
warning, rather than silently creating a broken setup where output
operations would fail.
This checks whether a file descriptor is valid and opened in a mode that
allows writing (O_WRONLY or O_RDWR). This is useful when we want to
verify that inherited FDs can actually be used for output operations
before dup'ing them.
The helper explicitly handles O_PATH file descriptors, which cannot be
used for I/O operations and thus are never writable.
The ASSERT_SIGNAL macro uses a fixed variable name, `_r`. This prevents
nesting the macro (like ASSERT_SIGNAL(ASSERT_SIGNAL(...))), as the inner
instance would shadow the outer instance's variable.
Switch to using the UNIQ_T helper to generate unique variable names at
each expansion level. This allows the macro to be used recursively,
which is required for upcoming regression tests regarding signal
handling logic.
Commit 88fce09 modified the mount_bind() function, causing it to perform
arithmetic on the uid_shift parameter. However, it performs this
arithmetic even when uid_shift was UID_INVALID, which was not intended.
This typically occurred when mount_custom() was called for a simple bind
mount without user namespaces (and thus no rootidmap mount option).
This arithmetic (e.g., uid_shift + m->destination_uid) then wraps
around, resulting in the invalid ID 4294967295 ((uid_t)-1).
This bug manifests for users running systemd-nspawn with
--link-journal=host and --volatile=yes (but without --private-users),
causing systemd-tmpfiles to fail.
Make mount_bind() robust by checking if uid_shift is valid before using
it in arithmetic. If it is UID_INVALID, it defaults to a shift of 0 for
the ownership calculation, restoring correct behavior for plain bind
mounts while preserving the intended logic for ID-mapped mounts.
Fixes: #39714
This adds a new `Hostname=` option to the [DHCPServerStaticLease]
section in .network files, allowing an administrator to assign a
specific hostname to a client receiving a static lease.
We automatically select the correct DHCP option to use based on the
format of the provided string:
- Single DNS labels are sent as Option 12.
- Names with multiple DNS labels are sent as Option 81 in wire format.
Fixes: #39634
Add testcase_btrfs_compression() to verify that btrfs partitions with
Compression= and CopyFiles= directives work correctly.
The test verifies the fix for issue #39584, where mkfs.btrfs would fail
with "ERROR: --compression must be used with --rootdir" when repart
tried to create compressed btrfs filesystems.
The test creates a partition definition with Format=btrfs,
Compression=zstd, and CopyFiles=, then validates:
1. systemd-repart output shows "Rootdir from:" and "Compress:",
confirming that the --rootdir code path is used
2. mkfs.btrfs is invoked with both --compress and --rootdir options
3. The file is successfully copied to the filesystem
4. Compression is actually applied (verified via compsize output
containing "zstd")
When a btrfs partition is configured with both Compression= and
CopyFiles=, we need to ensure files are copied during filesystem
creation using mkfs.btrfs --rootdir, rather than copying files
afterwards via loop device mounting.
This is required because mkfs.btrfs can only apply compression settings
when files are provided via --rootdir during filesystem creation. If we
format the filesystem first and then mount it to copy files, the
compression setting is meaningless.
Modify the partition_needs_populate() condition to force the --rootdir
code path when the format is btrfs and compression is requested.
This ensures that partition_populate_directory() runs and creates a
temporary directory with the files, which is then passed to
make_filesystem() as the root parameter, allowing mkfs.btrfs to create
the filesystem with compression applied.
Fixes: https://github.com/systemd/systemd/issues/39584
mkfs.btrfs requires that the --compress option be used together with
--rootdir, as compression only makes sense in that context (because
compression is not a persistent setting).
Right now, If --compress is specified without --rootdir, mkfs.btrfs
fails with:
ERROR: --compression must be used with --rootdir
This can occur when repart is configured with Compression= but the
partition populate logic doesn't use the --rootdir code path (eg. when
using loop device mounting to copy files after mkfs).
Add a defensive check to skip compression and emit a user-friendly
warning when compression is requested but no root directory is
provided. The warning message references the repart directive names
(Compression= and CopyFiles=) rather than low-level mkfs options to
help users understand the requirement.
This prevents crashes but doesn't enable compression, that requires
ensuring the --rootdir code path is used, which it currently is not and
will be addressed in the next patch.
Fixes: https://github.com/systemd/systemd/issues/39584
`systemctl show`'s `--timestamp` flag is supposed to reformat all
timestamp-based properties. However, the logic for detecting these
properties was incomplete and only checked if the name ended in
Timestamp.
Expand the check to explicitly include some non-"timestamp" named
properties that really are timestamps.
Fixes: https://github.com/systemd/systemd/issues/39282
Commit 38748596f0 ("core: Make DelegateNamespaces= work for user
managers with CAP_SYS_ADMIN") refactored the logic for when an
unprivileged process should create a new user namespace for sandboxing.
This refactor inadvertently removed a check (`params->runtime_scope !=
RUNTIME_SCOPE_USER`) that differentiated between system services and user
services.
This causes a regression in rootless containers where systemd runs
unprivileged. When starting a system service (like `dbus-broker`) that
uses sandboxing features (eg. with `PrivateTmp=yes`), systemd now
incorrectly creates a new, minimal `PRIVATE_USERS_SELF` namespace.
This new namespace only maps UID/GID 0. When dbus-broker attempts to
drop privileges to the `dbus` user (GID 81), the `setresgid(81, 81, 81)`
call fails because GID 81 is not mapped.
Restore the check to ensure that the special unprivileged sandboxing
logic is only applied to user services, as was the original intent.
System services in a rootless context will now correctly run in the
container's main user namespace, where all necessary UIDs/GIDs are
mapped.
Fixes: https://github.com/systemd/systemd/issues/39563
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2391343
When parsing an absolute time specification like `hh:mm` for the
`shutdown` command, the code interprets a time in the past as "tomorrow
at this time". It currently implements this by adding a fixed 24-hour
duration (`USEC_PER_DAY`) to the timestamp.
This assumption breaks across DST transitions, as the day might not be
24 hours long. This can cause the shutdown to be scheduled at the wrong
time (typically off by one hour in either direction).
Change the logic to perform calendar arithmetic instead of timestamp
arithmetic. If the calculated time is in the past, we increment
`tm.tm_mday` and call `mktime_or_timegm_usec()` a second time.
This delegates all date normalization logic to `mktime()`, which
correctly handles all edge cases, including DST transitions, month-end
rollovers, and leap years.
Fixes: https://github.com/systemd/systemd/issues/39232
Previously:
1. last_error wouldn't be updated with errors from is_dir;
2. We'd always issue a stat(), even for binaries without execute;
3. We used stat() instead of access(), which is cheaper.
This change avoids all of those, by only checking inside X_OK-positive
case whether access() works on the path with an extra slash appended.
Thanks to Lennart for the suggestion.
Imagine $PATH /a:/b. There is an echo command at /b/echo. Under this
configuration, this works fine:
% systemd-run --user --scope echo .
Running scope as unit: run-rfe98e0574b424d63a641644af511ff30.scope
.
However, if I do `mkdir /a/echo`, this happens:
% systemd-run --user --scope echo .
Running scope as unit: run-rcbe9369537ed47f282ee12ce9f692046.scope
Failed to execute: Permission denied
We check whether the resulting file is executable for the performing
user, but of course, most directories are anyway, since that's needed to
list within it. As such, another is_dir() check is needed prior to
considering the search result final.
Another approach might be to check S_ISREG, but there may be more gnarly
edge cases there than just eliminating this obviously pathological
example, so let's just do this for now.
Arch recently upgraded systemd to 245.6. Shortly afterwards, users began
reporting[0] that systemd detected an ordering cycle, and they were
unable to log in. The reason they were unable to log in was because of
ordering cycle resolution:
[...]
systemd[1]: sysinit.target: Job systemd-tmpfiles-setup.service/start deleted to break ordering cycle starting with sysinit.target/start
systemd[1]: sysinit.target: Job systemd-update-done.service/start deleted to break ordering cycle starting with sysinit.target/start
systemd[1]: sysinit.target: Job systemd-journal-catalog-update.service/start deleted to break ordering cycle starting with sysinit.target/start
systemd[1]: sysinit.target: Job local-fs.target/start deleted to break ordering cycle starting with sysinit.target/start
systemd[1]: sysinit.target: Job systemd-tmpfiles-setup.service/start deleted to break ordering cycle starting with sysinit.target/start
[...]
Whether the resolution did the right thing here or not is a longer-term
discussion, but in the interim we should at least make this distinction
between automount dependencies and mount dependencies clearer in the
documentation, so that users and distribution maintainers know what's
acceptable. In this case Arch actually backed out b3d7aef5 entirely and
released a new version due to the confusion.
Also see https://github.com/systemd/systemd-stable/issues/69.
0: https://bugs.archlinux.org/task/66908
Suppose a service has WatchdogSec set to 2 seconds in its unit file. I
then start the service and WatchdogUSec is set correctly:
% systemctl --user show psi-notify -p WatchdogUSec
WatchdogUSec=2s
Now I call `sd_notify(0, "WATCHDOG_USEC=10000000")`. The new timer seems
to have taken effect, since I only send `WATCHDOG=1` every 4 seconds,
and systemd isn't triggering the watchdog handler. However, `systemctl
show` still shows WatchdogUSec as 2s:
% systemctl --user show psi-notify -p WatchdogUSec
WatchdogUSec=2s
This seems surprising, since this "original" watchdog timer isn't the
one taking effect any more. This patch makes it so that we instead
display the new watchdog timer after sd_notify(WATCHDOG_USEC):
% systemctl --user show psi-notify -p WatchdogUSec
WatchdogUSec=10s
Fixes#15726.
5238d9a83a renames this to exit-status, but systemd.service was not
updated.
The rest of the doc seems a bit inconsistent in its use of the terms
"exit code" and "exit status", but it's not that confusing, so leave
those alone for now.
proot provides userspace-powered emulation of chroot and mount --bind,
lending it to be used on environments without unprivileged user
namespaces, or in otherwise restricted environments like Android.
In order to achieve this, proot makes use of the kernel's ptrace()
facility, which we can use in order to detect its presence. Since it
doesn't use any kind of namespacing, including PID namespacing, we don't
need to do any tricks when trying to get the tracer's metadata.
For our purposes, proot is listed as a "container", since we mostly use
this also as the bucket for non-container-but-container-like
technologies like WSL. As such, it seems like a good fit for this
section as well.
Inside format_bytes, we return NULL if the value is UINT64_MAX. This
makes some kind of sense where this has some other semantic meaning than
being a value, but in this case the value is both a.) not the default
(so we definitely want to display it), and b.) means "infinity" (or
"max" in cgroup terminology).
This patch adds a small wrapper around format_bytes that can be used for
these cases, to avoid the following situation:
[root@tangsanjiao ~]# cat /sys/fs/cgroup/workload.slice/memory.low
max
[root@tangsanjiao ~]# systemctl show workload.slice -p MemoryLow
MemoryLow=infinity
[root@tangsanjiao ~]# systemctl status workload.slice | grep low:
Memory: 14.9G (low: (null))
After the patch:
[root@tangsanjiao ~]# systemctl status workload.slice | grep low:
Memory: 15.1G (low: infinity)
This is the most basic consumer of the new systemd-vs-kernel checker,
both acting as a reasonable standalone exerciser of the code, and also
as a way for easy inspection of deviations from systemd internal state.
We currently don't have any mitigations against another privileged user
on the system messing with the cgroup hierarchy, bringing the system out
of line with what we've set in systemd. We also don't have any real way
to surface this to the user (we do have logs, but you have to know to
look in the first place).
There are a few possible solutions:
1. Maintaining our own cgroup tree with the new fsopen API and having a
read-only copy for everyone else. However, there are some
complications on this front, and this may be infeasible in some
environments. I'd rate this as a longer term effort that's tangential
to this patch.
2. Actively checking for changes with {fa,i}notify and changing them
back afterwards to match our configuration again. This is also
possible, but it's also good to have a way to do passive monitoring
of the situation without taking hard action. Also, currently daemons
like senpai do actually need to modify the tree behind systemd's
back (although hopefully this should be more integrated soon).
This patch implements another option, where one can, on demand, monitor
deviations in cgroup memory configuration from systemd's internal state.
Currently the only consumer is `systemd-analyze dump`, but the interface
is generic enough that it can also be exposed elsewhere later (for
example, over D-Bus).
Currently only memory limit style properties are supported, but later I
also plan to expand this out to other properties that systemd should
have ultimate control over.
A later version of the DefaultMemory{Low,Min} patch changed these to
require explicitly setting memory_foo_set, but we only set that in
load-fragment, not dbus-cgroup.
Without these, we may fall back to either DefaultMemoryFoo or
CGROUP_LIMIT_MIN when we really shouldn't.
This is an oversight from https://github.com/systemd/systemd/pull/12332.
Sadly the tests didn't catch it since it requires a real cgroup
hierarchy to see, and it wasn't seen in prod since we're only currently
using DefaultMemoryLow, not DefaultMemoryMin. :-(
Chris Down