Don't unload LKRG at shutdown

[adrelanos]

https://github.com/openwall/lkrg/blob/main/scripts/bootup/systemd/lkrg.service currently uses:

• Before=[...] shutdown.target
• ExecStop=/sbin/modprobe -v -r p_lkrg

I don't see what this would be good for? Could even be a security issue to unload LKRG?

System shutdown works well even if LKRG doesn ot get unload. If it's not needed then it's best avoided for simplicity.

Maybe related: https://github.com/openwall/lkrg/issues/108

[morfikov]

I didn't see that either, but some people want to reload the module and need the service to do that. Like they couldn't just use the modprobe command directly. :)

[adrelanos]

Seems like an insufficient rationale. For unload/reload of the kernel module it would be possible to implement this as sudo systemctl reload lkrg. ExecReload=

Related: the usefulness of the lkrg systemd unit existing at all has been in questioned here: https://github.com/openwall/lkrg/issues/108#issuecomment-886230431

[solardiz]

Our current instructions in the "Installation" and "Uninstalling and upgrading" sections of README rely on the current behavior, and make uninstall would currently unload LKRG via:

elif [ "$1" == "uninstall" ]; then
	echo -e "       ${P_GREEN}Stopping ${P_YL}lkrg.service${P_NC}"
	systemctl stop lkrg.service

So we should not simply drop these lines from scripts/bootup/systemd/lkrg.service without also revising the instructions and perhaps the above script.

[Adam-pi3]

@solardiz @adrelanos @morfikov should we close this issue?

[morfikov]

Probably.

[solardiz]

I think let's close for now. We can revisit later. We're considering additions to LKRG that could affect the decision-making here, so deciding on this issue now is premature.

[solardiz]

CC: @mrl5 (who is working on #197 now)

Just to have this recorded in here:

Could even be a security issue to unload LKRG?

Actually, yes. If a user process (an exploit) can stay running until late enough in system shutdown that LKRG is unloaded, it can then attack the kernel without LKRG's protection and introduce a persistent backdoor into the system (e.g., modify something in the userland) so that the attacker has access upon next bootup. I didn't check whether with our current systemd unit settings a user process can stay running until late enough (but I guess so) or maybe be SIGKILL'ed first.

So this is probably something we'll need to change. Once #197 is merged, for OpenRC as well.

I wonder if we reasonably can detect whether the system is being shut down, and only unload LKRG when the service is being stopped not as part of system shutdown sequence. With SysV init, I suppose we'd check the runlevel. What are its equivalents with systemd and OpenRC?

We're considering additions to LKRG that could affect the decision-making here, so deciding on this issue now is premature.

FWIW, I meant us possibly introducing an optional userspace service running along with the kernel module. That could be a reason against completely dropping our own systemd unit, and ditto for other init systems. However, while a decision to drop these now would be premature, we can nevertheless look into ways to avoid unloading LKRG on system shutdown.

Re-opening the issue so that we don't forget to revisit it and at least decide on it for real.

[mrl5]

@Adam-pi3 what was the motivation behind creating dedicated systemd service for lkrg (in commit https://github.com/lkrg-org/lkrg/commit/96721f15ecc6718d2f78d0989bba6702a67f2eee) instead of relying on https://www.freedesktop.org/software/systemd/man/modules-load.d.html?

my point here is that it turns out that with dedicated systemd/OpenRC service one must remember about few important things like load order as well as mentioned side effects of service being stopped just before shutdown - maybe dedicated service for loading a kernel module is over-engineering?

edit: for more context check this comment: https://github.com/lkrg-org/lkrg/pull/197#issuecomment-1175522750

[solardiz]

@mrl5 To your question (even though directed at Adam), I think the pros and cons were not specifically considered at the time. See also the comments at https://github.com/lkrg-org/lkrg/issues/108#issuecomment-968357020

[solardiz]

I wonder if we reasonably can detect whether the system is being shut down, and only unload LKRG when the service is being stopped not as part of system shutdown sequence.

I notice we already have:

Conflicts=shutdown.target

So maybe this already does the trick for shutdown, but maybe not for other "equivalent" targets? Maybe we need:

Conflicts=reboot.target poweroff.target halt.target shutdown.target

We also have:

Before=systemd-sysctl.service
Before=sysinit.target shutdown.target

Why do we list shutdown.target here? Maybe we need to drop it, @morfikov (as you're credited in the commit introducing those lines)?

I didn't test any of this for whether LKRG gets unloaded on reboot/poweroff/halt/shutdown; someone in here might want to.

[morfikov]

It's about the ExecStop line. Just comment it out or remove it altogether.

The dependencies you mentioned are for proper handling of the service, since lkrg service is started early on boot and the defaults don't apply in this case (on boot or shutdown). You can read more about it here.

[solardiz]

@morfikov Thanks, however this does not get us anywhere.

Of course, it's about ExecStop, however we want LKRG to be unloaded when the service is stopped, just not when it is stopped as part of system shutdown (or equivalent), and removing that line would break that. If we do, we could as well ditch the systemd unit altogether, which is within consideration.

Regarding the dependencies, I asked specifically about inclusion of shutdown.target in Before. The link you referenced appears to be about properly creating a new .target unit, which we don't.

[solardiz]

@morfikov Anyway, I don't mean to spend your time on (re-)researching this about shutdown.target in Before now. So if you don't recall, that's fine.

[morfikov]

No, it's a general thing to add:

Before=shutdown.target
Conflicts=shutdown.target

when a service has DefaultDependencies=no .There's lots of places in systemd manual to read about it, for instance here: https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Default%20Dependencies

Basically when you specify After and/or Before, the unit will be started in some specific order, and on shutdown the unit will be stopped also in some order, usually in reverse.

I'm not sure whether removing shutdown.target from Before and Conflicts will prevent the unit from stopping on shutdown. Someone has to confirm this.

[solardiz]

Thanks, @morfikov. Curiously, systemd has RefuseManualStop, but here we need "allow only manual stop".

[solardiz]

Looks like the runlevel command (traditional for SysV init) exists even with systemd, and produces the expected output of N 3 on a normally booted system. Perhaps it'd return 6 or such in the second field during shutdown? If so, we can check that inside the ExecStop command and make the actual action conditional.

[solardiz]

We went for checking runlevel for OpenRC in #197. Perhaps we can do the same for systemd. @morfikov Maybe you want to test this and contribute a PR? Thanks!

[morfikov]

I'm busy recently, so I can't help you with testing.