[systemd-devel] [PATCH] networkd: do not change kernel forwarding parameters when IPForwarding is unset

[Zbigniew Jędrzejewski-Szmek]

On Fri, May 01, 2015 at 02:05:11PM -0700, Nick Owens wrote:
> From: Nick Owens <mischief at offblast.org>
> 
> ---
>  man/systemd.network.xml        |  6 +++---
>  src/network/networkd-link.c    | 24 ++++++++++++------------
>  src/network/networkd-network.c |  2 ++
>  src/network/networkd.h         |  1 +
>  4 files changed, 18 insertions(+), 15 deletions(-)
> 
> the following patch should address https://bugs.freedesktop.org/show_bug.cgi?id=89509
> let me know if there is a problem with it.
> 
> sorry for the dup email tom et al., i had tried to send this email before subscribing to the list.

Based on the discussion on the bug tracker, Lennart is OK with this
change. I think it makes sense too.

Please add some commit message though, explaining the rationale for the change
and the link 'https://bugs.freedesktop.org/show_bug.cgi?id=89509'.

> 
> diff --git a/man/systemd.network.xml b/man/systemd.network.xml
> index 4be9d13..c5958e3 100644
> --- a/man/systemd.network.xml
> +++ b/man/systemd.network.xml
> @@ -356,7 +356,7 @@
>            <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
> +          the routing table. The argument can be unset, a boolean, or the
Generally we don't specify that settings can be left unset. There are no
obligatory settings at all afaik. So there's no need to change this part.

>            values <literal>ipv4</literal> or <literal>ipv6</literal>,
>            which only enables IP forwarding for the specified address
>            family. This controls the
> @@ -365,8 +365,8 @@
>            <filename>net.ipv6.conf.<interface>.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>
> +          for details about sysctl options). By default, if unset, the IP
> +          forwarding parameters of the interface will not be changed.
And you can simplify this last sentence: "If unset, IP forwarding
parameters of the interface will not be changed."

>  
>            <para>Note: unless this option is turned on, no IP
>            forwarding is done on this interface, even if this is
> diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
> index 0f9a1cd..6e7bade 100644
> --- a/src/network/networkd-link.c
> +++ b/src/network/networkd-link.c
> @@ -1488,16 +1488,14 @@ static int link_set_ipv4_forward(Link *link) {
>          bool b;
>          int r;
>  
> -        b = link_ipv4_forward_enabled(link);
> -
> -        p = strjoina("/proc/sys/net/ipv4/conf/", link->ifname, "/forwarding");
> -        r = write_string_file_no_create(p, one_zero(b));
> -        if (r < 0)
> -                log_link_warning_errno(link, r, "Cannot configure IPv4 forwarding for interface %s: %m", link->ifname);
> -
> -        if (b) {
> +        if (link->network->ip_forward != ADDRESS_FAMILY_UNSET) {
>                  _cleanup_free_ char *buf = NULL;
>  
> +                p = strjoina("/proc/sys/net/ipv4/conf/", link->ifname, "/forwarding");
> +                r = write_string_file_no_create(p, one_zero(link_ipv4_forward_enabled));
> +                if (r < 0)
> +                        log_link_warning_errno(link, r, "Cannot configure IPv4 forwarding for interface %s: %m", link->ifname);
> +
>                  /* 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
> @@ -1525,10 +1523,12 @@ static int link_set_ipv6_forward(Link *link) {
>          const char *p = NULL;
>          int r;
>  
> -        p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/forwarding");
> -        r = write_string_file_no_create(p, one_zero(link_ipv6_forward_enabled(link)));
> -        if (r < 0)
> -                log_link_warning_errno(link, r, "Cannot configure IPv6 forwarding for interface: %m");
> +        if (link->network->ip_forwarding != ADDRESS_FAMILY_UNSET) {
> +                p = strjoina("/proc/sys/net/ipv6/conf/", link->ifname, "/forwarding");
> +                r = write_string_file_no_create(p, one_zero(link_ipv6_forward_enabled(link)));
> +                if (r < 0)
> +                        log_link_warning_errno(link, r, "Cannot configure IPv6 forwarding for interface: %m");
> +        }
>  
>          return 0;
>  }
> diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c
> index 4d4972e..db36332 100644
> --- a/src/network/networkd-network.c
> +++ b/src/network/networkd-network.c
> @@ -109,6 +109,8 @@ static int network_load_one(Manager *manager, const char *filename) {
>  
>          network->link_local = ADDRESS_FAMILY_IPV6;
>  
> +        network->ip_forward = ADDRESS_FAMILY_UNSET;
I think it would better to use _ADDRESS_FAMILY_BOOLEAN_INVALID instead
of introducing an new enum value.

> +
>          r = config_parse(NULL, filename, file,
>                           "Match\0"
>                           "Link\0"
> diff --git a/src/network/networkd.h b/src/network/networkd.h
> index 4b13d4a..f4aefc1 100644
> --- a/src/network/networkd.h
> +++ b/src/network/networkd.h
> @@ -60,6 +60,7 @@ typedef enum AddressFamilyBoolean {
>          ADDRESS_FAMILY_IPV4 = 1,
>          ADDRESS_FAMILY_IPV6 = 2,
>          ADDRESS_FAMILY_YES = 3,
> +        ADDRESS_FAMILY_UNSET = 4,
>          _ADDRESS_FAMILY_BOOLEAN_MAX,
>          _ADDRESS_FAMILY_BOOLEAN_INVALID = -1,
>  } AddressFamilyBoolean;

Zbyszek