Merge pull request #1879 from poettering/networkd-forward

stop managing per-interface IP forwarding settings

6 files changed, 179 insertions, 110 deletions

diff --git a/man/systemd.network.xml b/man/systemd.network.xml
index 5994869d97..e6dedb027d 100644
--- a/man/systemd.network.xml
+++ b/man/systemd.network.xml
@@ -363,29 +363,28 @@
         </varlistentry>
         <varlistentry>
           <term><varname>IPForward=</varname></term>
-          <listitem><para>Configures IP forwarding for the network
-          interface. If enabled, incoming packets on the network
-          interface will be forwarded to other interfaces according to
-          the routing table. Takes either a boolean argument, or the
-          values <literal>ipv4</literal> or <literal>ipv6</literal>,
-          which only enables IP forwarding for the specified address
-          family, or <literal>kernel</literal>, which preserves existing sysctl settings.
-          This controls the
-          <filename>net.ipv4.conf.&lt;interface&gt;.forwarding</filename>
-          and
-          <filename>net.ipv6.conf.&lt;interface&gt;.forwarding</filename>
-          sysctl options of the network interface (see <ulink
+          <listitem><para>Configures IP packet forwarding for the
+          system. If enabled, incoming packets on any network
+          interface will be forwarded to any other interfaces
+          according to the routing table. Takes either a boolean
+          argument, or the values <literal>ipv4</literal> or
+          <literal>ipv6</literal>, which only enable IP packet
+          forwarding for the specified address family.  This controls
+          the <filename>net.ipv4.ip_forward</filename> and
+          <filename>net.ipv6.conf.all.forwarding</filename> sysctl
+          options of the network interface (see <ulink
           url="https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt">ip-sysctl.txt</ulink>
           for details about sysctl options). Defaults to
           <literal>no</literal>.</para>
 
-         <para>Note: unless this option is turned on, or set to <literal>kernel</literal>,
-          no IP forwarding is done on this interface, even if this is
-          globally turned on in the kernel, with the
-          <filename>net.ipv4.ip_forward</filename>,
-          <filename>net.ipv4.conf.all.forwarding</filename>, and
-          <filename>net.ipv6.conf.all.forwarding</filename> sysctl
-          options.</para>
+          <para>Note: this setting controls a global kernel option,
+          and does so one way only: if a network that has this setting
+          enabled is set up the global setting is turned on.  However,
+          it is never turned off again, even after all networks with
+          this setting enabled are shut down again.</para>
+
+          <para>To allow IP packet forwarding only between specific
+          network interfaces use a firewall.</para>
           </listitem>
         </varlistentry>
         <varlistentry>
diff --git a/src/basic/fileio.c b/src/basic/fileio.c
index be6e327690..10aacdc56d 100644
--- a/src/basic/fileio.c
+++ b/src/basic/fileio.c
@@ -78,6 +78,7 @@ static int write_string_file_atomic(const char *fn, const char *line, bool enfor
 
 int write_string_file(const char *fn, const char *line, WriteStringFileFlags flags) {
         _cleanup_fclose_ FILE *f = NULL;
+        int q, r;
 
         assert(fn);
         assert(line);
@@ -85,30 +86,58 @@ int write_string_file(const char *fn, const char *line, WriteStringFileFlags fla
         if (flags & WRITE_STRING_FILE_ATOMIC) {
                 assert(flags & WRITE_STRING_FILE_CREATE);
 
-                return write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+                r = write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+                if (r < 0)
+                        goto fail;
+
+                return r;
         }
 
         if (flags & WRITE_STRING_FILE_CREATE) {
                 f = fopen(fn, "we");
-                if (!f)
-                        return -errno;
+                if (!f) {
+                        r = -errno;
+                        goto fail;
+                }
         } else {
                 int fd;
 
                 /* We manually build our own version of fopen(..., "we") that
                  * works without O_CREAT */
                 fd = open(fn, O_WRONLY|O_CLOEXEC|O_NOCTTY);
-                if (fd < 0)
-                        return -errno;
+                if (fd < 0) {
+                        r = -errno;
+                        goto fail;
+                }
 
                 f = fdopen(fd, "we");
                 if (!f) {
+                        r = -errno;
                         safe_close(fd);
-                        return -errno;
+                        goto fail;
                 }
         }
 
-        return write_string_stream(f, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        r = write_string_stream(f, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        if (r < 0)
+                goto fail;
+
+        return 0;
+
+fail:
+        if (!(flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE))
+                return r;
+
+        f = safe_fclose(f);
+
+        /* OK, the operation failed, but let's see if the right
+         * contents in place already. If so, eat up the error. */
+
+        q = verify_file(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE));
+        if (q <= 0)
+                return r;
+
+        return 0;
 }
 
 int read_one_line_file(const char *fn, char **line) {
@@ -139,15 +168,41 @@ int read_one_line_file(const char *fn, char **line) {
         return 0;
 }
 
-int verify_one_line_file(const char *fn, const char *line) {
-        _cleanup_free_ char *value = NULL;
-        int r;
+int verify_file(const char *fn, const char *blob, bool accept_extra_nl) {
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_free_ char *buf = NULL;
+        size_t l, k;
 
-        r = read_one_line_file(fn, &value);
-        if (r < 0)
-                return r;
+        assert(fn);
+        assert(blob);
+
+        l = strlen(blob);
+
+        if (accept_extra_nl && endswith(blob, "\n"))
+                accept_extra_nl = false;
+
+        buf = malloc(l + accept_extra_nl + 1);
+        if (!buf)
+                return -ENOMEM;
 
-        return streq(value, line);
+        f = fopen(fn, "re");
+        if (!f)
+                return -errno;
+
+        /* We try to read one byte more than we need, so that we know whether we hit eof */
+        errno = 0;
+        k = fread(buf, 1, l + accept_extra_nl + 1, f);
+        if (ferror(f))
+                return errno > 0 ? -errno : -EIO;
+
+        if (k != l && k != l + accept_extra_nl)
+                return 0;
+        if (memcmp(buf, blob, l) != 0)
+                return 0;
+        if (k > l && buf[l] != '\n')
+                return 0;
+
+        return 1;
 }
 
 int read_full_stream(FILE *f, char **contents, size_t *size) {
diff --git a/src/basic/fileio.h b/src/basic/fileio.h
index 5f2c941498..95e8698941 100644
--- a/src/basic/fileio.h
+++ b/src/basic/fileio.h
@@ -34,6 +34,7 @@ typedef enum {
         WRITE_STRING_FILE_CREATE = 1,
         WRITE_STRING_FILE_ATOMIC = 2,
         WRITE_STRING_FILE_AVOID_NEWLINE = 4,
+        WRITE_STRING_FILE_VERIFY_ON_FAILURE = 8,
 } WriteStringFileFlags;
 
 int write_string_stream(FILE *f, const char *line, bool enforce_newline);
@@ -43,7 +44,7 @@ int read_one_line_file(const char *fn, char **line);
 int read_full_file(const char *fn, char **contents, size_t *size);
 int read_full_stream(FILE *f, char **contents, size_t *size);
 
-int verify_one_line_file(const char *fn, const char *line);
+int verify_file(const char *fn, const char *blob, bool accept_extra_nl);
 
 int parse_env_file(const char *fname, const char *separator, ...) _sentinel_;
 int load_env_file(FILE *f, const char *fname, const char *separator, char ***l);
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 00c57b2960..c37532bb73 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -111,16 +111,26 @@ static bool link_ipv4_forward_enabled(Link *link) {
         if (!link->network)
                 return false;
 
+        if (link->network->ip_forward == _ADDRESS_FAMILY_BOOLEAN_INVALID)
+                return false;
+
         return link->network->ip_forward & ADDRESS_FAMILY_IPV4;
 }
 
 static bool link_ipv6_forward_enabled(Link *link) {
+
+        if (!socket_ipv6_is_supported())
+                return false;
+
         if (link->flags & IFF_LOOPBACK)
                 return false;
 
         if (!link->network)
                 return false;
 
+        if (link->network->ip_forward == _ADDRESS_FAMILY_BOOLEAN_INVALID)
+                return false;
+
         return link->network->ip_forward & ADDRESS_FAMILY_IPV6;
 }
 
@@ -147,6 +157,10 @@ bool link_ipv6_accept_ra_enabled(Link *link) {
 }
 
 static IPv6PrivacyExtensions link_ipv6_privacy_extensions(Link *link) {
+
+        if (!socket_ipv6_is_supported())
+                return _IPV6_PRIVACY_EXTENSIONS_INVALID;
+
         if (link->flags & IFF_LOOPBACK)
                 return _IPV6_PRIVACY_EXTENSIONS_INVALID;
 
@@ -1847,55 +1861,43 @@ static int link_enter_join_netdev(Link *link) {
 }
 
 static int link_set_ipv4_forward(Link *link) {
-        const char *p = NULL, *v;
         int r;
 
-        if (link->flags & IFF_LOOPBACK)
+        if (!link_ipv4_forward_enabled(link))
                 return 0;
 
-        if (link->network->ip_forward == _ADDRESS_FAMILY_BOOLEAN_INVALID)
-                return 0;
+        /* We propagate the forwarding flag from one interface to the
+         * global setting one way. This means: as long as at least one
+         * interface was configured at any time that had IP forwarding
+         * enabled the setting will stay on for good. We do this
+         * primarily to keep IPv4 and IPv6 packet forwarding behaviour
+         * somewhat in sync (see below). */
 
-        p = strjoina("/proc/sys/net/ipv4/conf/", link->ifname, "/forwarding");
-        v = one_zero(link_ipv4_forward_enabled(link));
-
-        r = write_string_file(p, v, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, v) > 0)
-                        return 0;
-
-                log_link_warning_errno(link, r, "Cannot configure IPv4 forwarding for interface %s: %m", link->ifname);
-        }
+        r = write_string_file("/proc/sys/net/ipv4/ip_forward", "1", WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
+                log_link_warning_errno(link, r, "Cannot turn on IPv4 packet forwarding, ignoring: %m");
 
         return 0;
 }
 
 static int link_set_ipv6_forward(Link *link) {
-        const char *p = NULL, *v = NULL;
         int r;
 
-        /* Make this a NOP if IPv6 is not available */
-        if (!socket_ipv6_is_supported())
-                return 0;
-
-        if (link->flags & IFF_LOOPBACK)
-                return 0;
-
-        if (link->network->ip_forward == _ADDRESS_FAMILY_BOOLEAN_INVALID)
+        if (!link_ipv6_forward_enabled(link))
                 return 0;
 
-        p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/forwarding");
-        v = one_zero(link_ipv6_forward_enabled(link));
+        /* On Linux, the IPv6 stack does not not know a per-interface
+         * packet forwarding setting: either packet forwarding is on
+         * for all, or off for all. We hence don't bother with a
+         * per-interface setting, but simply propagate the interface
+         * flag, if it is set, to the global flag, one-way. Note that
+         * while IPv4 would allow a per-interface flag, we expose the
+         * same behaviour there and also propagate the setting from
+         * one to all, to keep things simple (see above). */
 
-        r = write_string_file(p, v, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, v) > 0)
-                        return 0;
-
-                log_link_warning_errno(link, r, "Cannot configure IPv6 forwarding for interface: %m");
-        }
+        r = write_string_file("/proc/sys/net/ipv6/conf/all/forwarding", "1", WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
+                log_link_warning_errno(link, r, "Cannot configure IPv6 packet forwarding, ignoring: %m");
 
         return 0;
 }
@@ -1906,25 +1908,16 @@ static int link_set_ipv6_privacy_extensions(Link *link) {
         const char *p = NULL;
         int r;
 
-        /* Make this a NOP if IPv6 is not available */
-        if (!socket_ipv6_is_supported())
-                return 0;
-
         s = link_ipv6_privacy_extensions(link);
-        if (s == _IPV6_PRIVACY_EXTENSIONS_INVALID)
+        if (s < 0)
                 return 0;
 
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/use_tempaddr");
-        xsprintf(buf, "%u", link->network->ipv6_privacy_extensions);
-
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
+        xsprintf(buf, "%u", (unsigned) link->network->ipv6_privacy_extensions);
 
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot configure IPv6 privacy extension for interface: %m");
-        }
 
         return 0;
 }
@@ -1940,23 +1933,21 @@ static int link_set_ipv6_accept_ra(Link *link) {
         if (link->flags & IFF_LOOPBACK)
                 return 0;
 
-        p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/accept_ra");
+        if (!link->network)
+                return 0;
 
-        /* we handle router advertisments ourselves, tell the kernel to GTFO */
-        r = write_string_file(p, "0", 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, "0") > 0)
-                        return 0;
+        p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/accept_ra");
 
+        /* We handle router advertisments ourselves, tell the kernel to GTFO */
+        r = write_string_file(p, "0", WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot disable kernel IPv6 accept_ra for interface: %m");
-        }
 
         return 0;
 }
 
 static int link_set_ipv6_dad_transmits(Link *link) {
-        char buf[DECIMAL_STR_MAX(unsigned) + 1];
+        char buf[DECIMAL_STR_MAX(int) + 1];
         const char *p = NULL;
         int r;
 
@@ -1967,27 +1958,24 @@ static int link_set_ipv6_dad_transmits(Link *link) {
         if (link->flags & IFF_LOOPBACK)
                 return 0;
 
+        if (!link->network)
+                return 0;
+
         if (link->network->ipv6_dad_transmits < 0)
                 return 0;
 
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/dad_transmits");
+        xsprintf(buf, "%i", link->network->ipv6_dad_transmits);
 
-        xsprintf(buf, "%u", link->network->ipv6_dad_transmits);
-
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
-
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot set IPv6 dad transmits for interface: %m");
-        }
 
         return 0;
 }
 
 static int link_set_ipv6_hop_limit(Link *link) {
-        char buf[DECIMAL_STR_MAX(unsigned) + 1];
+        char buf[DECIMAL_STR_MAX(int) + 1];
         const char *p = NULL;
         int r;
 
@@ -1998,21 +1986,18 @@ static int link_set_ipv6_hop_limit(Link *link) {
         if (link->flags & IFF_LOOPBACK)
                 return 0;
 
+        if (!link->network)
+                return 0;
+
         if (link->network->ipv6_hop_limit < 0)
                 return 0;
 
         p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/hop_limit");
+        xsprintf(buf, "%i", link->network->ipv6_hop_limit);
 
-        xsprintf(buf, "%u", link->network->ipv6_hop_limit);
-
-        r = write_string_file(p, buf, 0);
-        if (r < 0) {
-                /* If the right value is set anyway, don't complain */
-                if (verify_one_line_file(p, buf) > 0)
-                        return 0;
-
+        r = write_string_file(p, buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE);
+        if (r < 0)
                 log_link_warning_errno(link, r, "Cannot set IPv6 hop limit for interface: %m");
-        }
 
         return 0;
 }
diff --git a/src/network/networkd-util.c b/src/network/networkd-util.c
index df091393f6..2545621a93 100644
--- a/src/network/networkd-util.c
+++ b/src/network/networkd-util.c
@@ -79,10 +79,18 @@ int config_parse_address_family_boolean_with_kernel(
         assert(rvalue);
         assert(data);
 
+        /* This function is mostly obsolete now. It simply redirects
+         * "kernel" to "no". In older networkd versions we used to
+         * distuingish IPForward=off from IPForward=kernel, where the
+         * former would explicitly turn off forwarding while the
+         * latter would simply not touch the setting. But that logic
+         * is gone, hence silently accept the old setting, but turn it
+         * to "no". */
+
         s = address_family_boolean_from_string(rvalue);
         if (s < 0) {
                 if (streq(rvalue, "kernel"))
-                        s = _ADDRESS_FAMILY_BOOLEAN_INVALID;
+                        s = ADDRESS_FAMILY_NO;
                 else {
                         log_syntax(unit, LOG_ERR, filename, line, 0, "Failed to parse IPForward= option, ignoring: %s", rvalue);
                         return 0;
diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c
index e588681b86..bde3c7c3cf 100644
--- a/src/test/test-fileio.c
+++ b/src/test/test-fileio.c
@@ -363,6 +363,26 @@ static void test_write_string_file_no_create(void) {
         unlink(fn);
 }
 
+static void test_write_string_file_verify(void) {
+        _cleanup_free_ char *buf = NULL, *buf2 = NULL;
+        int r;
+
+        assert_se(read_one_line_file("/proc/cmdline", &buf) >= 0);
+        assert_se((buf2 = strjoin(buf, "\n", NULL)));
+
+        r = write_string_file("/proc/cmdline", buf, 0);
+        assert_se(r == -EACCES || r == -EIO);
+        r = write_string_file("/proc/cmdline", buf2, 0);
+        assert_se(r == -EACCES || r == -EIO);
+
+        assert_se(write_string_file("/proc/cmdline", buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE) == 0);
+        assert_se(write_string_file("/proc/cmdline", buf2, WRITE_STRING_FILE_VERIFY_ON_FAILURE) == 0);
+
+        r = write_string_file("/proc/cmdline", buf, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_AVOID_NEWLINE);
+        assert_se(r == -EACCES || r == -EIO);
+        assert_se(write_string_file("/proc/cmdline", buf2, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_AVOID_NEWLINE) == 0);
+}
+
 static void test_load_env_file_pairs(void) {
         char fn[] = "/tmp/test-load_env_file_pairs-XXXXXX";
         int fd;
@@ -419,6 +439,7 @@ int main(int argc, char *argv[]) {
         test_write_string_stream();
         test_write_string_file();
         test_write_string_file_no_create();
+        test_write_string_file_verify();
         test_load_env_file_pairs();
 
         return 0;