dbus.service unit file doesn't specify explicit service "Type" in the [Service] section. Hence dbus.service is of Type=simple which is default service type. This means that systemd assumes the service is started immediately after it is forked off. Once systemd starts dbus then it immediately tries to connect to it. However, systemd connects to dbus in synchronous* manner. So if systemd calls synchronouns dbus API in order to connect and in response (or because it was about to do it anyway) dbus-daemon calls some code (e.g. getgrouplist()) that could potentially end up waiting on systemd we created a deadlock (think of glibc call that eventually calls NSS module that wants to socket activate system service in order to proceed). To partially avoid deadlocks I propose to make dbus service of "Type=notify" so we send READY=1 notification to systemd only after we parsed the configuration, just before entering main loop. Frankly I am not sure if this change would eliminate deadlocks entirely (probably not), but I recon that it would be the step in the good direction anyway. * This is not entirely true. There has been work done on sd-bus side to get rid of blocking/waiting on dbus-daemon but these enhancements are not readily available to users (to appear in systemd-237).
(In reply to Michal Sekletar from comment #0) > To partially avoid deadlocks I propose to make dbus service of "Type=notify" > so we send READY=1 notification to systemd only after we parsed the > configuration, just before entering main loop. Sounds like a good idea - I thought we did this anyway, tbh. If you need a short-term workaround to check whether this would fix a particular deadlock, you can run the dbus-daemon with --fork and configure it for Forking mode - that's how it works under non-systemd init systems. There is a bit more initialization after it has double-forked, but only the sorts of things that, if they fail, mean you have far bigger problems than occasional deadlocks.
Testing patches now; they're pretty simple.
(In reply to Michal Sekletar from comment #0) > So if systemd > calls synchronouns dbus API in order to connect and in response (or because > it was about to do it anyway) dbus-daemon calls some code (e.g. > getgrouplist()) that could potentially end up waiting on systemd we created > a deadlock (think of glibc call that eventually calls NSS module that wants > to socket activate system service in order to proceed). I don't think Type=notify can prevent this. dbus-daemon currently needs to consult NSS for every incoming connection anyway, to find out its complete list of supplementary groups. On a suitably recent Linux kernel (4.14+), implementing the feature request in Bug #103737 might avoid that.
Created attachment 136730 [details] [review] Proposed patch FWIW, here is what I put together, only "compile tested"
(In reply to Michal Sekletar from comment #4) > FWIW, here is what I put together, only "compile tested" I think I prefer my version, which is also only compile-tested so far (I'll need to do a more production-ready build to try this out, which is ... far from instantaneous).
Created attachment 136732 [details] [review] bus: Don't pass systemd environment variables to activated services --- With NotifyAccess=main (the default for Type=notify) they wouldn't be allowed to write to the notify socket anyway, but it's nicer to not tell them to.
Created attachment 136733 [details] [review] bus: Notify systemd when we are ready
Created attachment 136734 [details] [review] bus: Also tell systemd when we're reloading --- Might as well!
Created attachment 136735 [details] [review] bus: Also tell systemd before we shut down
Comment on attachment 136732 [details] [review] bus: Don't pass systemd environment variables to activated services Review of attachment 136732 [details] [review]: ----------------------------------------------------------------- A question. ::: bus/activation.c @@ +853,5 @@ > + _dbus_hash_table_remove_string (activation->environment, "JOURNAL_STREAM"); > + _dbus_hash_table_remove_string (activation->environment, "LISTEN_FDNAMES"); > + _dbus_hash_table_remove_string (activation->environment, "LISTEN_FDS"); > + _dbus_hash_table_remove_string (activation->environment, "LISTEN_PID"); > + _dbus_hash_table_remove_string (activation->environment, "NOTIFY_SOCKET"); §(ENVIRONMENT VARIABLES IN SPAWNED PROCESSES) in systemd.exec(5) lists some other variables, like INVOCATION_ID, which look like they shouldn’t be propagated. Should they be removed too?
Comment on attachment 136733 [details] [review] bus: Notify systemd when we are ready Review of attachment 136733 [details] [review]: ----------------------------------------------------------------- r- ::: bus/bus.c @@ +1025,4 @@ > goto failed; > } > > + _dbus_daemon_report_ready (); bus_context_new() is called from bus/test.c too — do we really want to notify from the tests? Also, looking at bus/main.c, the reload pipe and SIGTERM/SIGHUP handlers are only installed after bus_context_new() returns. Don’t we want them to be set up before notifying systemd? ::: bus/dbus.service.in @@ +4,4 @@ > Requires=dbus.socket > > [Service] > +Type=notify Maybe also explicitly set NotifyAccess=main? I realise this is the default, but we want to make it explicit that this should not be NotifyAccess=all, for example. ::: bus/systemd-user/dbus.service.in @@ +4,4 @@ > Requires=dbus.socket > > [Service] > +Type=notify Same here.
Comment on attachment 136734 [details] [review] bus: Also tell systemd when we're reloading Review of attachment 136734 [details] [review]: ----------------------------------------------------------------- r+
(In reply to Philip Withnall from comment #10) > §(ENVIRONMENT VARIABLES IN SPAWNED PROCESSES) in systemd.exec(5) lists some > other variables, like INVOCATION_ID, which look like they shouldn’t be > propagated. Should they be removed too? I think you don't want to unset blindly all of them. INVOCATION_ID is useful for clients supporting structured logging. Then you can distinguish between log messages generated by multiple service runs using INVOCATION_ID. Long story short, you probably want to unset only sd_notify related env variables, because they only clutter child's environment and are useless anyway as dbus.service *doesn't* have NotifyAccess=all.
Comment on attachment 136735 [details] [review] bus: Also tell systemd before we shut down Review of attachment 136735 [details] [review]: ----------------------------------------------------------------- r+
(In reply to Michal Sekletar from comment #13) > INVOCATION_ID is > useful for clients supporting structured logging. Then you can distinguish > between log messages generated by multiple service runs using INVOCATION_ID. The situation in which dbus-daemon can have subprocesses is that it starts an activatable service (a system service if it's the system bus, or a session/user service if it's the session bus), and the activatable service does not have SystemdService set up to delegate its activation to systemd, so it's run directly as a subprocess of (a subprocess of) dbus-daemon. For system services it doesn't actually matter, because dbus-daemon launches traditional system services via the setuid activation helper, which clears the environment (except for a couple of variables). For session services it does matter. If `dbus-daemon --session` starts an activatable session service - perhaps something like gconf - as its own child process, is it correct to say that gconf somehow belongs to that invocation of dbus-daemon? I don't know the answer to that. If activation was delegated to systemd (as is now done for e.g. gnome-terminal-server) then it would have its own, parallel INVOCATION_ID.
(In reply to Philip Withnall from comment #10) > §(ENVIRONMENT VARIABLES IN SPAWNED PROCESSES) in systemd.exec(5) lists some > other variables These are potentially applicable in this process structure: systemd --user \- dbus-daemon --session \- (babysitter process) \- traditional activated service (gconfd or something) Other than INVOCATION_ID and the ones I already reset: PATH, LANG, USER, LOGNAME, HOME, SHELL: These are properties of the user or system configuration, so they are equally true for the activated service and should not be reset. XDG_RUNTIME_DIR, XDG_SESSION_ID, XDG_SEAT, XDG_VTNR: These are properties of the session to which the dbus-daemon and the activated service belong, so they are equally true for the activated service and should not be reset. TERM: Not set for dbus-daemon, which is not attached to a terminal, so not applicable. MANAGERPID: It's still an equally true fact that this pid is the pid of `systemd --user`, even for an activated service that was not *directly* started by systemd, so I'd say don't reset. WATCHDOG_*: Not set for dbus-daemon, which doesn't participate in the watchdog, so not applicable. MAINPID, SERVICE_RESULT, EXIT_CODE, EXIT_STATUS: Not set for ExecStart, so not applicable.
(In reply to Philip Withnall from comment #11) > bus_context_new() is called from bus/test.c too — do we really want to > notify from the tests? They shouldn't inherit a NOTIFY_SOCKET, so sd_notify() will just fail anyway, which we ignore. > Also, looking at bus/main.c, the reload pipe and SIGTERM/SIGHUP handlers are > only installed after bus_context_new() returns. Don’t we want them to be set > up before notifying systemd? Fair point. Michal's patch put it just before entering the main loop, which might be a better place. > Maybe also explicitly set NotifyAccess=main? I realise this is the default, > but we want to make it explicit that this should not be NotifyAccess=all, > for example. Yeah, perhaps. Thanks for pre-reviewing this so that I can restrict "real system" testing to a version that is basically already approved!
(In reply to Michal Sekletar from comment #13) > Long story short, you probably want to unset only sd_notify related env > variables Justification for the ones I unset: JOURNAL_STREAM: When we start traditional activatable services, we give them a Journal stream of their own, to avoid dbus-daemon being "blamed" for their stdout/stderr in the Journal; so inheriting JOURNAL_STREAM is misleading. We could set a new value that reflects the new streams, but I think that's a job for a separate commit. LISTEN_*: This is dbus-daemon's listening socket, not the activatable service's (but we pass TRUE to sd_listen_fds() so these get unset anyway). NOTIFY_SOCKET: We don't want the activatable service to be telling systemd that dbus-daemon is ready (plus it isn't allowed to, because of NotifyAccess).
Created attachment 137403 [details] [review] [1/4 v2] bus: Don't pass systemd environment variables to activated services --- Justify the ones we do and don't want to clear, in comments. I have not changed the behaviour since the previous version of this patch.
Created attachment 137404 [details] [review] [2/4 v2] bus: Notify systemd when we are ready --- Don't notify ready until just before we run the main loop. Explicitly set NotifyAccess=main (which is the default anyway).
Comment on attachment 137403 [details] [review] [1/4 v2] bus: Don't pass systemd environment variables to activated services Review of attachment 137403 [details] [review]: ----------------------------------------------------------------- ++ ::: bus/activation.c @@ +861,5 @@ > + * the activated service, should not be reset > + * - TERM, WATCHDOG_*: Should not be set for dbus-daemon, so not applicable > + * - MAINPID, SERVICE_RESULT, EXIT_CODE, EXIT_STATUS: Not set for ExecStart, > + * so not applicable > + * - INVOCATION_ID: TODO: Do we want to clear this or not? It isn't clear. Perhaps discuss the INVOCATION_ID with upstream systemd to get their ideas about the intent behind it?
Comment on attachment 137403 [details] [review] [1/4 v2] bus: Don't pass systemd environment variables to activated services Review of attachment 137403 [details] [review]: ----------------------------------------------------------------- ::: bus/activation.c @@ +861,5 @@ > + * the activated service, should not be reset > + * - TERM, WATCHDOG_*: Should not be set for dbus-daemon, so not applicable > + * - MAINPID, SERVICE_RESULT, EXIT_CODE, EXIT_STATUS: Not set for ExecStart, > + * so not applicable > + * - INVOCATION_ID: TODO: Do we want to clear this or not? It isn't clear. (To be clear, I’m happy with this being merged with the TODO comment in place.)
Comment on attachment 137404 [details] [review] [2/4 v2] bus: Notify systemd when we are ready Review of attachment 137404 [details] [review]: ----------------------------------------------------------------- r+
Fixed in git for 1.13.4, and I've asked the systemd mailing list whether we should additionally remove the INVOCATION_ID from the environment.
Created attachment 138222 [details] [review] bus: Clear INVOCATION_ID when carrying out traditional activation We weren't sure whether this one should be inherited or not, so I asked on systemd-devel, and Lennart thought it shouldn't.
Comment on attachment 138222 [details] [review] bus: Clear INVOCATION_ID when carrying out traditional activation Review of attachment 138222 [details] [review]: ----------------------------------------------------------------- r+, thanks for this attention to detail
Fixed (again) in git for 1.13.4
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.