[systemd-devel] [PATCH] networkd: don't touch global forwarding setting

[Benedikt Morbach]

this inevitably leads to race conditions and also means that IPForward=yes on
one interface is equivalent to setting it on _every_ interface.
(except when it isn't, see below)

Suppose you have two networks

 * /etc/systemd/network/eth0.network
    [Match]
    Name=eth0

    [Network]
    Address=10.0.0.1/24
    IPForward=yes

 * /etc/systemd/network/eth1.network
    [Match]
    Name=eth1

    [Network]
    Address=192.168.0.1/24

Depending on which interface gets brought up first, the forwarding situation
looks different.
If eth0 gets brought up first, it enables forwarding for itself and globally.
Then eth1 goes up and gets forwarding disabled as per the default.
However, if eth0 comes up later, it will enable forwarding globally, so in this
case both interfaces have forwarding enabled in the end.

To fix that, stop setting the global forwarding setting. This means that
IPForward needs to be set on all interfaces which should do forwarding
individually.

Also respect the default value from sysfs so that it can still be turned on
globally if desired.

systemd could still default to IPForward=no by using the default sysctl.conf
---
 src/network/networkd-link.c       | 23 -----------------------
 src/network/networkd-network.c    | 15 +++++++++++++++
 units/systemd-networkd.service.in |  2 +-
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index ab115a0..933ed2f 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -1495,29 +1495,6 @@ static int link_set_ipv4_forward(Link *link) {
         if (r < 0)
                 log_link_warning_errno(link, r, "Cannot configure IPv4 forwarding for interface %s: %m", link->ifname);
 
-        if (b) {
-                _cleanup_free_ char *buf = NULL;
-
-                /* If IP forwarding is turned on for this interface,
-                 * then propagate this to the global setting. Given
-                 * that turning this on has side-effects on other
-                 * fields, we'll try to avoid doing this unless
-                 * necessary, hence check the previous value
-                 * first. Note that we never turn this option off
-                 * again, since all interfaces we manage do not do
-                 * forwarding anyway by default, and ownership rules
-                 * of this control are so unclear. */
-
-                r = read_one_line_file("/proc/sys/net/ipv4/ip_forward", &buf);
-                if (r < 0)
-                        log_link_warning_errno(link, r, "Cannot read /proc/sys/net/ipv4/ip_forward: %m");
-                else if (!streq(buf, "1")) {
-                        r = write_string_file_no_create("/proc/sys/net/ipv4/ip_forward", "1");
-                        if (r < 0)
-                                log_link_warning_errno(link, r, "Cannot write /proc/sys/net/ipv4/ip_forward: %m");
-                }
-        }
-
         return 0;
 }
 
diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c
index 64c1b9c..e3f2299 100644
--- a/src/network/networkd-network.c
+++ b/src/network/networkd-network.c
@@ -22,6 +22,7 @@
 #include <ctype.h>
 #include <net/if.h>
 
+#include "fileio.h"
 #include "conf-files.h"
 #include "conf-parser.h"
 #include "util.h"
@@ -33,6 +34,7 @@
 static int network_load_one(Manager *manager, const char *filename) {
         _cleanup_network_free_ Network *network = NULL;
         _cleanup_fclose_ FILE *file = NULL;
+        _cleanup_free_ char *buf = NULL;
         char *d;
         Route *route;
         Address *address;
@@ -124,6 +126,19 @@ static int network_load_one(Manager *manager, const char *filename) {
         if (r < 0)
                 return r;
 
+        /* If IP forwarding is turned on globally, then respect
+         * that default in networkd as well. */
+        r = read_one_line_file("/proc/sys/net/ipv4/ip_forward", &buf);
+        if (r < 0)
+                log_warning("Cannot read /proc/sys/net/ipv4/ip_forward");
+        else if (streq(buf, "1"))
+                network->ip_forward |= ADDRESS_FAMILY_IPV4;
+        r = read_one_line_file("/proc/sys/net/ipv6/conf/all/forwarding", &buf);
+        if (r < 0)
+                log_warning("Cannot read /proc/sys/net/ipv6/conf/all/forwarding");
+        else if (streq(buf, "1"))
+                network->ip_forward |= ADDRESS_FAMILY_IPV6;
+
         /* IPMasquerade=yes implies IPForward=yes */
         if (network->ip_masquerade)
                 network->ip_forward |= ADDRESS_FAMILY_IPV4;
diff --git a/units/systemd-networkd.service.in b/units/systemd-networkd.service.in
index 5a91b8e..527f6c0 100644
--- a/units/systemd-networkd.service.in
+++ b/units/systemd-networkd.service.in
@@ -12,7 +12,7 @@ ConditionCapability=CAP_NET_ADMIN
 DefaultDependencies=no
 # dbus.service can be dropped once on kdbus, and systemd-udevd.service can be
 # dropped once tuntap is moved to netlink
-After=systemd-udevd.service dbus.service network-pre.target systemd-sysusers.service
+After=systemd-udevd.service dbus.service network-pre.target systemd-sysctl.service systemd-sysusers.service
 Before=network.target multi-user.target shutdown.target
 Conflicts=shutdown.target
 Wants=network.target
-- 
2.3.3