[PATCH 0/8] improve systemd service

[PATCH 0/8] improve systemd service

[Christoph Anton Mitterer]

Hey.

This is a first series of patches that tries to improve the included
`nftables.service`.

It contains the (hopefully) less controversial stuff I’d like to do. O:-)

The main idea is to make things more hardened as it should be for loading
firewall rules.


[PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit
might need some more discussion:

1. Instead of the whole logic I’ve addeed we could also choose to simply never
   do anything on stop, i.e. not seeting `ExecStop=` at all, which would mean
   that even on `systemctl stop nftables.service`, the ruleset isn’t flushed.

   Might even better prevent accidentally stopping the firewall; and writing
   `nft flush ruleset` instead would be even shorter.


2. If we keep the basic idea as proposed in the patch, there's still:
   a) Do you want my slightly awkward multi-line syntax with `\n\` line
      terminators or would you rather have anything in one line or something
      else?

      A problematic thing about the multi-line syntax is that it seems
      impossible to write e.g. the shell’s `;;` on a single line with only
      leading whitespace, as systemd then ignores that as a comment. See [0].
      Might be error prone with future changes.

   b) systemd sets some default PATH (which is even an compile option, IIRC)
      that usually contains /usr/loca/[s]bin.
      Because of that it’s IMO safer to use absolute pathnames for sh and
      systemctl (just in case a user made some local overrides).

      Maybe it would be even better to set a fixde `ExecSearchPath=` (wihout
      /u/local), IIRC that also exports PATH, which would then be inherited by
      `nft`, should that ever exec some binary via PATH resoluiton.

      I have chose to *not* use @sbindir@ (respectively add a @bindir@) for sh
      and systemctl because these aren’t shipped by nftables, and I think if any
      distro would really use something different, patching the file would be
      completely upon them.

      While I’ve been in favour of usr-merge, I still can’t write `/usr/bin/sh`.
      ;-)

      The perfectionist in me would want to add a `unset -v job_type`, to
      prevent that from being exported to `nft`, just in case anyone would have
      configured/override his systemd to export a env var of the same name.


A planned 2nd series would focus on hardening with things like the already
present `ProtectSystem=`.
Quite some more sandboxing seetings seem to be usable with this and IMO units
should generally try to set as many as (reasonably) possible.

I’ll probably send a mail first with proposals on what could be hardened and
what people would think about the various points.


Last but not least, one idea would have been to set:
  WorkingDirectory=/etc

If no further `--includepath` is configured, this would cause relative pathnames
starting with `./` in nft’s `include` statement to use the same path as relative
pathnames without `./` – at least when the default `--includepath` is kept at
`/etc`.
No strong opinion myself, whther that would be better or not. Anyone?

It wouldn’t be backwards compatible, if someone used the service with it’s
current (current) working dir (which is /), and specified absolute pathnames as
relative ones.


Cheers,
Chris.

[0] https://github.com/systemd/systemd/issues/39332

[PATCH 1/9] tools: don’t set options whose values match their defaults

[Christoph Anton Mitterer]

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index 2ac7e6fd..e694f2f7 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -10,7 +10,6 @@ ConditionPathExists=@pkgsysconfdir@/rules/main.nft
 [Service]
 Type=oneshot
 RemainAfterExit=yes
-StandardInput=null
 ProtectSystem=full
 ProtectHome=true
 ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
-- 
2.51.0

[PATCH 2/9] tools: use the same pair of boolean literals

[Christoph Anton Mitterer]

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index e694f2f7..edc7c831 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -11,7 +11,7 @@ ConditionPathExists=@pkgsysconfdir@/rules/main.nft
 Type=oneshot
 RemainAfterExit=yes
 ProtectSystem=full
-ProtectHome=true
+ProtectHome=yes
 ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
 ExecReload=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
 ExecStop=@sbindir@/nft flush ruleset
-- 
2.51.0

[PATCH 3/9] tools: include further `Documentation=` URIs

[Christoph Anton Mitterer]

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index edc7c831..ca2ef684 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -1,6 +1,6 @@
 [Unit]
 Description=nftables static rule set
-Documentation=man:nftables.service(8)
+Documentation=man:nftables.service(8) man:nft(8) https://wiki.nftables.org
 Wants=network-pre.target
 Before=network-pre.target shutdown.target
 Conflicts=shutdown.target
-- 
2.51.0

[PATCH 4/9] tools: reorder options

[Christoph Anton Mitterer]

This groups related options, orders options/groups by importance and
separates sections/groups with empty lines.

In `[Unit]` this groups general, dependency and condition options.
In `[Service]` this groups general, execution and hardening options.

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index ca2ef684..15c3b5da 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -1,20 +1,26 @@
 [Unit]
 Description=nftables static rule set
 Documentation=man:nftables.service(8) man:nft(8) https://wiki.nftables.org
+
 Wants=network-pre.target
 Before=network-pre.target shutdown.target
 Conflicts=shutdown.target
 DefaultDependencies=no
+
 ConditionPathExists=@pkgsysconfdir@/rules/main.nft
 
+
 [Service]
 Type=oneshot
 RemainAfterExit=yes
-ProtectSystem=full
-ProtectHome=yes
+
 ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
 ExecReload=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
 ExecStop=@sbindir@/nft flush ruleset
 
+ProtectSystem=full
+ProtectHome=yes
+
+
 [Install]
 WantedBy=sysinit.target
-- 
2.51.0

[PATCH 5/9] tools: depend on `sysinit.target`

[Christoph Anton Mitterer]

As of systemd 258.1 and as per systemd.service(5), `DefaultDependencies=yes`
implies for non-instanced service units:
Requires=sysinit.target
Conflicts=shutdown.target
Before=shutdown.target
After=sysinit.target basic.target

Previous commit messages don’t indicate why `DefaultDependencies=no` was set,
presumably because systemd.special(7) suggests for `sysinit.target` that
services pulled in by it – as is the case for `nftables.service` – “should
declare DefaultDependencies=no and specify all their dependencies manually”.

It’s however unclear why the dependencies on `sysinit.target` and `basic.target`
were dropped.

This commit adds the ones on the former, assuming that at least `@sbindir@/nft`
and `@pkgsysconfdir@/rules/main.nft` are required. As per bootup(7),
`sysinit.target` pulls in `local-fs.target`, which should make these available.

`local-fs.target` might have been enough, but some other services pulled in by
`sysinit.target` (the “various low-level services: udevd, tmpfiles, random seed,
sysctl, ...”) seem to be reasonable, too.

`basic.target`, which as per systemd.special(7) and bootup(7) seems to
additionally pull in in particular `timers.target`, `paths.target` and
`sockets.target` seems rather not required for nftables and is thus not added by
this commit.

`remote-fs.target` can’t be pulled in either, as `nftables.service` shall run
before any networking is brought up.

It shall be noted, that this doesn’t render that `WantedBy=sysinit.target`
superfluous.
The (added) dependencies of `nftables.service` merely specify what nftables
needs to be run – the `[Install]` on the other hand specifies “who” pulls in
`nftables.service`.

Typically, networking is configured in later targets, usually
`multi-user.target`.
In particular, `emergency.target` should run without any networking, so nothing
needs to be done for that. Similarly for `rescue.target` and while networking
might be started, nftables rules being loaded before it is not ensured.
Scenarios where networking is already brought up during the initramfs are not
covered by this and would have no nftables rules loaded.

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index 15c3b5da..8388ae68 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -3,7 +3,9 @@ Description=nftables static rule set
 Documentation=man:nftables.service(8) man:nft(8) https://wiki.nftables.org
 
 Wants=network-pre.target
+Requires=sysinit.target
 Before=network-pre.target shutdown.target
+After=sysinit.target
 Conflicts=shutdown.target
 DefaultDependencies=no
 
-- 
2.51.0

[PATCH 6/9] tools: don’t stop `nftables.service` (and flush the ruleset) on shutdown

[Christoph Anton Mitterer]

There doesn’t seem any compelling reason to stop the service on shutdown.
`nftables.service` is anyway a `Type=oneshot` service, so unless for some
extreme cases (like when the shutdown happens while service commands still
haven’t finished (and exited)), no processes from it should be left anyway.

At best, flushing the ruleset on shutdown, merely and needlessly executes yet
another `nft` process.
At worst it could flush the ruleset when networking is still running for
whatever reason.

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index 8388ae68..ea428ee7 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -4,9 +4,8 @@ Documentation=man:nftables.service(8) man:nft(8) https://wiki.nftables.org
 
 Wants=network-pre.target
 Requires=sysinit.target
-Before=network-pre.target shutdown.target
+Before=network-pre.target
 After=sysinit.target
-Conflicts=shutdown.target
 DefaultDependencies=no
 
 ConditionPathExists=@pkgsysconfdir@/rules/main.nft
-- 
2.51.0

[PATCH 7/9] tools: don’t stop `nftables.service` (and flush the ruleset) when isolating another unit

[Christoph Anton Mitterer]

The nftables ruleset shouldn’t be unexpectedly flushed, which might however
happen when `nftables.service` is stopped as a consequence of isolating another
unit.

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index ea428ee7..5f367a24 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -8,6 +8,8 @@ Before=network-pre.target
 After=sysinit.target
 DefaultDependencies=no
 
+IgnoreOnIsolate=yes
+
 ConditionPathExists=@pkgsysconfdir@/rules/main.nft
 
 
-- 
2.51.0

[PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Christoph Anton Mitterer]

In systemd, the `ExecStop=` command isn’t only executed on an actual dedicated
unit stop, but also, for example, on restart (which is a stop followed by a
start).

There’s a chance users either don’t know this or (accidentally) confuse restart
(and similar) with reload.
In this case, there would be a short but non-zero time window in which the
ruleset is flushed (and any firewall rules gone), which for security reasons
shouldn’t happen.

Even with the pure semantics of restart (and similar), there’s no good reason to
actually flush the ruleset (on stop) before reloading it again (on start),
because the start will already do the flushing.

This commit adds detection of the job type to the `ExecStop=` command and,
depending on that, either actually flushes the ruleset (for example if a
dedicated `systemctl stop nftables.service` was executed) or doesn’t do so.
The latter is the case, on restart respectively try-restart, which prints
however a message that actually no stop was performed (mostly intended for the
journal), as well as on any other job type (which currently shouldn’t happen).

In order to determine the job type, first the systemd job ID is determined from
the service’s `Job=`-property.
See also https://github.com/systemd/systemd/issues/39316.

The service is referenced via `%n`, which needs not to be (shell) quoted, as –
as of systemd 258.1 and as per systemd.unit(5) – “the "unit name prefix" must
consist of one or more valid characters (ASCII letters, digits, ":", "-", "_",
".", and "\")”.

Programs are invoked by absolute pathname to prevent any of their names being
used from `/usr/local/bin`, which systemd includes in its default PATH.

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index 5f367a24..0ad66b8c 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -19,7 +19,15 @@ RemainAfterExit=yes
 
 ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
 ExecReload=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
-ExecStop=@sbindir@/nft flush ruleset
+ExecStop=:/bin/sh -c 'job_type="$$( /usr/bin/systemctl show --property JobType --value "$$(/usr/bin/systemctl show --property Job --value %n)" )"\n\
+                      case "$${job_type}" in\n\
+                      (stop)\n\
+                       @sbindir@/nft flush ruleset;;\n\
+                      (restart|try-restart)\n\
+                       printf \'%%s: JobType is `%%s`, thus the stop is ignored.\' %n "$${job_type}" >&2;;\n\
+                      (*)\n\
+                       printf \'%%s: Unexpected JobType `%%s`.\' %n "$${job_type}" >&2; exit 1\n\
+                      esac'
 
 ProtectSystem=full
 ProtectHome=yes
-- 
2.51.0

[PATCH 9/9] tools: let the unit fail if the rules file is missing

[Christoph Anton Mitterer]

The idea is that if `nftables.service` is enabled it’s likely meant to load
firewall rules and shall thus rather fail loudly if that can’t be done because
the rules file is (perhaps accidentally) missing.
The default should be hardened/secure behaviour, so if someone actually wants
the conditional a systemd drop-in configuration should be used in that case.

If no rules shall be loaded by `nftables.service` it can simply be disabled (for
example, when shipped as part of a downstream package that also contains the
`nft` program).

Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
---
 tools/nftables.service.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/nftables.service.in b/tools/nftables.service.in
index 0ad66b8c..81082882 100644
--- a/tools/nftables.service.in
+++ b/tools/nftables.service.in
@@ -10,8 +10,6 @@ DefaultDependencies=no
 
 IgnoreOnIsolate=yes
 
-ConditionPathExists=@pkgsysconfdir@/rules/main.nft
-
 
 [Service]
 Type=oneshot
-- 
2.51.0

Re: [PATCH 0/8] improve systemd service

[Florian Westphal]

Christoph Anton Mitterer <mail@christoph.anton.mitterer.name> wrote:

[ CC Jan ]

> This is a first series of patches that tries to improve the included
> `nftables.service`.

Sir, this is netfilter-devel and not nftables-systemd-devel@.

> It contains the (hopefully) less controversial stuff I’d like to do. O:-)
> 
> The main idea is to make things more hardened as it should be for loading
> firewall rules.

I have no horse in this race but I don't want to have too many changes
to this thing.

I see Jans original service file) as convinience / ease-of-use contribution
not as something that should be maintained continuously.

As for your series:

Its waaaay to many tiny patches.  The first 4 patches could easily be
squashed into one without making it hard to review.

As for flush-on-shutdown: I see no need for that either.  Jan?

FTR, this is about:
https://patchwork.ozlabs.org/series/479245/mbox/

Re: [PATCH 4/9] tools: reorder options

[Jan Engelhardt]

On Friday 2025-10-24 04:08, Christoph Anton Mitterer wrote:

>This groups related options, orders options/groups by importance and
>separates sections/groups with empty lines.

This one feels unnecessary, and I do not think there is an officially
supported order of directives.
In addition, the file should try to stay simple enough that order
does not matter.

Re: [PATCH 5/9] tools: depend on `sysinit.target`

[Jan Engelhardt]

On Friday 2025-10-24 04:08, Christoph Anton Mitterer wrote:

>As of systemd 258.1 and as per systemd.service(5), `DefaultDependencies=yes`
>implies for non-instanced service units[...]

The explanation is sound, but long-winded. Feel free to shorten the
commit message to simply sy that no reason *for* DefDeps= is on record
when it should have.

I was a second-hand submitter, and did not question what the distros had
concocted up in their .service files. I acknowledge no additional thought was
given to DefDeps=no, and that's on me.

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Jan Engelhardt]

On Friday 2025-10-24 04:08, Christoph Anton Mitterer wrote:

>In systemd, the `ExecStop=` command isn’t only executed on an actual dedicated
>unit stop, but also, for example, on restart (which is a stop followed by a
>start).
>
>There’s a chance users either don’t know this or (accidentally) confuse restart
>(and similar) with reload.

I do not see room for a confusion, for I cannot imagine a Linux newbie in 2025
starting off with a sysvinit-based distro (good luck finding those)
and then deciding "yep, I'll edit nftables.service".

>@@ -19,7 +19,15 @@ RemainAfterExit=yes
> 
> ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
> ExecReload=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
>-ExecStop=@sbindir@/nft flush ruleset
>+ExecStop=:/bin/sh -c 'job_type="$$( /usr/bin/systemctl show --property JobType --value "$$(/usr/bin/systemctl show --property Job --value %n)" )"\n\
>+                      case "$${job_type}" in\n\
>+                      (stop)\n\
>+                       @sbindir@/nft flush ruleset;;\n\
>+                      (restart|try-restart)\n\
>+                       printf \'%%s: JobType is `%%s`, thus the stop is ignored.\' %n "$${job_type}" >&2;;\n\
>+                      (*)\n\
>+                       printf \'%%s: Unexpected JobType `%%s`.\' %n "$${job_type}" >&2; exit 1\n\
>+                      esac'

No, let's not do this.

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Florian Westphal]

Jan Engelhardt <ej@inai.de> wrote:
> >@@ -19,7 +19,15 @@ RemainAfterExit=yes
> > 
> > ExecStart=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
> > ExecReload=@sbindir@/nft 'flush ruleset; include "@pkgsysconfdir@/rules/main.nft"'
> >-ExecStop=@sbindir@/nft flush ruleset
> >+ExecStop=:/bin/sh -c 'job_type="$$( /usr/bin/systemctl show --property JobType --value "$$(/usr/bin/systemctl show --property Job --value %n)" )"\n\
> >+                      case "$${job_type}" in\n\
> >+                      (stop)\n\
> >+                       @sbindir@/nft flush ruleset;;\n\
> >+                      (restart|try-restart)\n\
> >+                       printf \'%%s: JobType is `%%s`, thus the stop is ignored.\' %n "$${job_type}" >&2;;\n\
> >+                      (*)\n\
> >+                       printf \'%%s: Unexpected JobType `%%s`.\' %n "$${job_type}" >&2; exit 1\n\
> >+                      esac'
> 
> No, let's not do this.

Agree, thanks Jan for reviewing.

Re: [PATCH 0/8] improve systemd service

[Christoph Anton Mitterer]

On Tue, 2025-10-28 at 17:33 +0100, Florian Westphal wrote:
> > This is a first series of patches that tries to improve the
> > included
> > `nftables.service`.
> 
> Sir, this is netfilter-devel and not nftables-systemd-devel@.

Uhm? nftables ships the .service file, none of this was about systemd
development itself, there doesn't seem to be a dedicated list for
nftables and no one complained about me posting nftables manpage
patches on netfilter-devel.


> I have no horse in this race but I don't want to have too many
> changes
> to this thing.
> 
> I see Jans original service file) as convinience / ease-of-use
> contribution
> not as something that should be maintained continuously.

Hope that I don't negatively affect anyone's use case for this, but if
the file isn't meant to be maintained, shouldn't it then rather be
dropped?
That would kinda encourage downstreams to think themselves what proper
firewall service file should do any whether any hardening or error-
prone behaviour is desired or not.



> Its waaaay to many tiny patches.  The first 4 patches could easily be
> squashed into one without making it hard to review.

Merely did this out of courtesy so that you could easier pick what you
want to have and what not... squashing them is always a matter of
minutes.


I'm absolutely fine if none of these patches gets merged, I merely
based them on the unit file I'm using for my cluster here.

Incidentally, I'd have considered the one that doesn't cause a short
flush on restart the most important one, so not sure if the others
alone make that big change...


Cheers,
Chris.

Re: [PATCH 4/9] tools: reorder options

[Christoph Anton Mitterer]

On Tue, 2025-10-28 at 18:15 +0100, Jan Engelhardt wrote:
> This one feels unnecessary, and I do not think there is an officially
> supported order of directives.
> In addition, the file should try to stay simple enough that order
> does not matter.

No there's no official order, I merely felt that keeping related
settings together (like the dependencies) makes things more readable.

Re: [PATCH 5/9] tools: depend on `sysinit.target`

[Christoph Anton Mitterer]

On Tue, 2025-10-28 at 18:19 +0100, Jan Engelhardt wrote:
> The explanation is sound, but long-winded. Feel free to shorten the
> commit message to simply sy that no reason *for* DefDeps= is on
> record
> when it should have.

Wenn I read the commit that added the file to netfilter, it didn't
mention why e.g. DefaultDependencies= were chosen as it was (and why
the values for it implies were e.g. not set).

I figured, if my patch would have gotten merged, even only a few months
from now, no would remember why I e.g. set
After=/Requires=sysinit.target *and* WantedBy=sysinit.target, but e.g.
not basic.target.

So IMO it's better to have a good rationale, than not.


Cheer,
Chris.

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Christoph Anton Mitterer]

On Tue, 2025-10-28 at 18:31 +0100, Jan Engelhardt wrote:
> On Friday 2025-10-24 04:08, Christoph Anton Mitterer wrote:
> > In systemd, the `ExecStop=` command isn’t only executed on an
> > actual dedicated
> > unit stop, but also, for example, on restart (which is a stop
> > followed by a
> > start).
> > 
> > There’s a chance users either don’t know this or (accidentally)
> > confuse restart
> > (and similar) with reload.
> 
> I do not see room for a confusion, for I cannot imagine a Linux
> newbie in 2025
> starting off with a sysvinit-based distro (good luck finding those)
> and then deciding "yep, I'll edit nftables.service".

That rationale, I don't understand. What does editing nftables.service
have to do with it?

The scenario I'm trying to "solve" is:
A user changes his nftables rules and wants to get them loaded.
Unknowingly or accidentally he does a
  systemctl restart nftables.service
rather than
  systemctl reload nftables.service

The former, with the current service file in git, will cause the
ExecStop= command to be executed and the ruleset to be flushed.
Even if the new rules are sound, there'll be a short time window in
which everything is open.
And if the rules contain a syntax error/etc. the stop implied by the
restart will still succeed but the (re)start will fail, leaving the
firewall even longer open.

One can assume now that all systemd users know about reload vs. restart
(and that nftables.service actually has a reload defined), but that
seems quite optimistic to me.

So I felt if nftables *does* ship an example .service, it can well ship
one that handles such things better and less error prone.
Unfortunately, I found no better/shorter means for doing so than the
way I've had proposed.


Cheers,
Chris.

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Jan Engelhardt]

On Wednesday 2025-10-29 01:41, Christoph Anton Mitterer wrote:
>That rationale, I don't understand. What does editing nftables.service
>have to do with it?
>
>The scenario I'm trying to "solve" is:
>A user changes his nftables rules and wants to get them loaded.
>Unknowingly or accidentally he does a
>  systemctl restart nftables.service
>rather than
>  systemctl reload nftables.service

>Even if the new rules are sound, there'll be a short time window in
>which everything is open.

"The universe will always build a better idiot"

before you know it they'll type `systemctl stop nftables; systemctl start
nftables`, nullifying the restart magic.

If you really need restart-safe behavior, then just delete the
ExecStop= line entirely (at the cost of not flushing the ruleset).
sysctl.service does not have ExecStop either, it also does not reset
sysctl settings.

Re: [PATCH 0/8] improve systemd service

[Florian Westphal]

Christoph Anton Mitterer <mail@christoph.anton.mitterer.name> wrote:
> On Tue, 2025-10-28 at 17:33 +0100, Florian Westphal wrote:
> > > This is a first series of patches that tries to improve the
> > > included
> > > `nftables.service`.
> > 
> > Sir, this is netfilter-devel and not nftables-systemd-devel@.
> 
> Uhm? nftables ships the .service file, none of this was about systemd
> development itself, there doesn't seem to be a dedicated list for
> nftables and no one complained about me posting nftables manpage
> patches on netfilter-devel.

I don't have the expertise to review systemd service files.

> > I see Jans original service file) as convinience / ease-of-use
> > contribution
> > not as something that should be maintained continuously.
> 
> Hope that I don't negatively affect anyone's use case for this, but if
> the file isn't meant to be maintained, shouldn't it then rather be
> dropped?

If this thing needs constant changes then yes, absolutely.

Re: [PATCH 0/8] improve systemd service

[Jan Engelhardt]

On Wednesday 2025-10-29 12:40, Florian Westphal wrote:
>> 
>> Hope that I don't negatively affect anyone's use case for this, but if
>> the file isn't meant to be maintained, shouldn't it then rather be
>> dropped?
>
>If this thing needs constant changes then yes, absolutely.

If that were the case, we should drop Linux like a hot potato.
Constantly changing!

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Christoph Anton Mitterer]

On Wed, 2025-10-29 at 11:07 +0100, Jan Engelhardt wrote:
> "The universe will always build a better idiot"
> 
> before you know it they'll type `systemctl stop nftables; systemctl
> start
> nftables`, nullifying the restart magic.

Well but here it's literally to be seen in the command, that there's
first a stop, then a start.
Which is - at least not that - obvious with restart, especially as
there are services which have no ExecStop=, as you say.


> If you really need restart-safe behavior, then just delete the
> ExecStop= line entirely (at the cost of not flushing the ruleset).
> sysctl.service does not have ExecStop either, it also does not reset
> sysctl settings.

Well, I had suggested that as an alternative in the first mail of the
series.

I think it would be better than what we have now, but may of course be
disliked by people who actually want to use the stop action to disable
any firewalling.
That's why I came up with the proposed solution that would give us
both.


But I merely proposed that for the benefit over everyone, so I'm
totally find with ending discussion at this point and simply leaving
things as is.


Cheers,
Chris.

Re: [PATCH 8/9] tools: flush the ruleset only on an actual dedicated unit stop

[Christoph Anton Mitterer]

Hey.

I guess none of these patches will be merged anyway (which is okay for
me :-) )... but just in case something would be picked after all,
please ping me first as I've made several minor changes/improvements in
my own version meanwhile.


In particular, this patch contained a rather grave error:

On Fri, 2025-10-24 at 04:08 +0200, Christoph Anton Mitterer wrote:
> +ExecStop=:/bin/sh -c 'job_type="$$( /usr/bin/systemctl show --
> property JobType --value "$$(/usr/bin/systemctl show --property Job -
> -value %n)" )"\n\
> +                      case "$${job_type}" in\n\
> +                      (stop)\n\
> +                       @sbindir@/nft flush ruleset;;\n\
> +                      (restart|try-restart)\n\
> +                       printf \'%%s: JobType is `%%s`, thus the stop
> is ignored.\' %n "$${job_type}" >&2;;\n\
> +                      (*)\n\
> +                       printf \'%%s: Unexpected JobType `%%s`.\' %n
> "$${job_type}" >&2; exit 1\n\
> +                      esac'


When ':' is used a executable prefix, '$$' is not the escaped form of
'$' (as it is without), but rather stays the literal '$$' (which in a
shell of course is the special parameter $ (the PID of the shell).


Cheers,
Chris.