[libvirt] [PATCH 1/3] Only check for IP forwarding, do not enable it

[Michal Privoznik]

On 15.10.2012 12:26, Benjamin Cama wrote:
> Enabling IP forwarding without the administrator's consent is bad. Only
> check for forwarding, do not enable it.
> ---
>  src/network/bridge_driver.c |   56 +++++++++++++++++++++++++++++++++----------
>  1 files changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index e1846ee..e3e8dc2 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1853,19 +1853,51 @@ networkReloadIptablesRules(struct network_driver *driver)
>      }
>  }
>  
> -/* Enable IP Forwarding. Return 0 for success, -1 for failure. */
> +#define SYSCTL_PATH "/proc/sys"
> +
> +/* Check for IP Forwarding. Return 0 if required family is enabled, -1 for failure. */
>  static int
> -networkEnableIpForwarding(bool enableIPv4, bool enableIPv6)
> +networkCheckIpForwarding(bool checkIPv4, bool checkIPv6)
>  {
>      int ret = 0;
> -    if (enableIPv4)
> -        ret = virFileWriteStr("/proc/sys/net/ipv4/ip_forward", "1\n", 0);
> -    if (enableIPv6 && ret == 0)
> -        ret = virFileWriteStr("/proc/sys/net/ipv6/conf/all/forwarding", "1\n", 0);
> -    return ret;
> -}
> +    char *buf = NULL;
>  
> -#define SYSCTL_PATH "/proc/sys"
> +    if (checkIPv4) {
> +        ret = virFileReadAll(SYSCTL_PATH "/net/ipv4/ip_forward", 2, &buf);
> +        if (ret != 2)
> +            goto error;
> +        if (STRNEQ(buf, "1\n")) {
> +            networkReportError(VIR_ERR_SYSTEM_ERROR,
> +                               _("IP forwarding is not enabled on your system"));

We dropped networkReportError and use virReportError instead. Moreover,
you miss "%s" there:

virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("blah"));

> +            goto cleanup;
> +        }
> +    }
> +    if (checkIPv6) {
> +        if (ret > 0)

This check is useless since VIR_FREE(NULL) is safe.

> +            VIR_FREE(buf);
> +        ret = virFileReadAll(SYSCTL_PATH "/net/ipv6/conf/all/forwarding", 2, &buf);
> +        if (ret != 2)
> +            goto error;
> +        if (STRNEQ(buf, "1\n")) {
> +            networkReportError(VIR_ERR_SYSTEM_ERROR,
> +                               _("IPv6 forwarding is not enabled on your system"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    VIR_FREE(buf);
> +    return 0;
> +
> +error:
> +    virReportSystemError(errno, "%s", _("cannot check for IP forwarding"));
> +cleanup:
> +    if (ret > 0) {
> +        VIR_FREE(buf);
> +        return -1;
> +    } else {
> +        return ret;
> +    }
> +}

We rather stick with flow:

static int
func(...)
{
  int ret = -1;

  ... <commands> ...
  if (cond) goto cleanup;
  ... <commands> ...

  ret = 0;
cleanup:
  return ret;
}

>  
>  static int
>  networkSetIPv6Sysctls(virNetworkObjPtr network)
> @@ -2140,11 +2172,9 @@ networkStartNetworkVirtual(struct network_driver *driver,
>      if (virNetDevSetOnline(network->def->bridge, 1) < 0)
>          goto err2;
>  
> -    /* If forwardType != NONE, turn on global IP forwarding */
> +    /* If forwardType != NONE, check for IP forwarding */
>      if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE &&
> -        networkEnableIpForwarding(v4present, v6present) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("failed to enable IP forwarding"));
> +        networkCheckIpForwarding(v4present, v6present) < 0) {
>          goto err3;
>      }
>  
> 

Well, I am not sure if we can do this. What would happen if some of our
users rely on this already? I mean, it's there since ages.

Michal