-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
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
nixos/rtkit: Disable debug logging and fix sd_notify #180102
base: master
Are you sure you want to change the base?
Conversation
systemd.services.rtkit-daemon = { | ||
environment.NOTIFY_SOCKET = "/fs/sd-notify"; | ||
serviceConfig = { | ||
BindPaths = "/run/systemd/notify:/proc/fs/sd-notify"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a terrible hack. Is this the right way to go considering this service is type dbus
, so sd_notify doesn't really change the status anyway?
See https://github.com/heftig/rtkit/blob/c295fa849f52b487be6433e69e08b46251950399/rtkit-daemon.service.in#L23
According to service docs the notification is only interesting in case of Type=notify
.
On fedora for example this works without log spam and without specific socket mapping.
Or from a different angle: What does having a "Status" line do for us in a service which is actively managed through a specified dbus topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sd_notify works with non-notify services as well. The reason to use Type=notify
is startup notifications, not whether sd_notify()
works. That is controlled by NotifyAccess=
.
The Status:
line is Status: "Supervising 0 threads of 0 processes of 0 users."
, which is the same as the debug message that is logged constantly.
Fixing the status line is pretty irrelevant to me, I don't care about it at all, I just implemented it so people who care about the info that is being logged constantly have a way of accessing it that isn't annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging into this more, adding notification won't influence the logs, since they're both sent anyway: https://github.com/heftig/rtkit/blob/c295fa849f52b487be6433e69e08b46251950399/rtkit-daemon.c#L1435-L1446
I think bumping up the log level is a good idea, but otherwise the behaviour should be either fixed upstream or patched. The whole chroot("/proc")
could be replaced with a strict filesystem protection at service level anyway which would unbreak the status notification. (disable do_chroot) But messing with mounting over random /proc
directories is just... yuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree but it was disabling chroot() vs messing with /proc and while I agree with you that this is very ugly, it's the solution that doesn't lower security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to make it more clear: I'm not happy with this solution at all but it shuts up the daemon while not supressing info that was available previously. And upstream does not seem to care at all: heftig/rtkit#26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replicate the chroot with TemporaryFileSystem and BindPaths and then disable the chroot the application creates itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this should work:
ProtectSystem=strict
PrivateDevices=true
InaccessiblePaths=/etc
TemporaryFileSystem=/var
BindPaths=/run/systemd/notify
SystemCallFilter=~@mount
(not tested)
Pull in a patch from a cleaner upstream PR to reduce logging. Slightly update the URLs used to download the existing patches to make it easier to find the relevant PRs. Re: NixOS#180102, heftig/rtkit#22
This change looks good to me because it provides a quick way to see how many threads rtkit thinks should be RT. Despite all the debug logging, rtkit still fails to do its one job on my system (see heftig/rtkit#13). I don't think it's too important exactly how the sd-notify socket is bind-mounted into a chroot. |
Description of changes
See: heftig/rtkit#22
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes