[libvirt] [PATCH v3] nwfilter: check for inverted ctdir

[Laine Stump]

On 05/14/2013 03:58 PM, Stefan Berger wrote:
> Linux netfilter at some point (Linux 2.6.39) inverted the meaning of the
> '--ctdir reply' and newer netfilter implementations now expect
> '--ctdir original' instead and vice-versa.
> We check for the kernel version and assume that all Linux kernels with version
> 2.6.39 have the newer inverted logic.
>
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>
> ---
>  v2->v3:
>   - using uname now to check for Linux kernel version number
>
>  v1->v2:
>   - using virSocketAddrParseIPv4
>
> ---
>  src/nwfilter/nwfilter_ebiptables_driver.c |   54 ++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> ===================================================================
> --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -27,6 +27,7 @@
>  #include <string.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
> +#include <sys/utsname.h>
>  
>  #include "internal.h"
>  
> @@ -85,6 +86,17 @@ static char *iptables_cmd_path;
>  static char *ip6tables_cmd_path;
>  static char *grep_cmd_path;
>  
> +/*
> + * --ctdir original vs. --ctdir reply's meaning was inverted in netfilter
> + * at some point (Linux 2.6.39)
> + */
> +enum ctdirStatus {
> +    CTDIR_STATUS_UNKNOWN    = 0,
> +    CTDIR_STATUS_CORRECTED  = 1,
> +    CTDIR_STATUS_OLD        = 2,
> +};
> +static enum ctdirStatus iptables_ctdir_corrected;
> +
>  #define PRINT_ROOT_CHAIN(buf, prefix, ifname) \
>      snprintf(buf, sizeof(buf), "libvirt-%c-%s", prefix, ifname)
>  #define PRINT_CHAIN(buf, prefix, ifname, suffix) \
> @@ -1262,6 +1274,17 @@ iptablesEnforceDirection(int directionIn
>                           virNWFilterRuleDefPtr rule,
>                           virBufferPtr buf)
>  {
> +    switch (iptables_ctdir_corrected) {
> +    case CTDIR_STATUS_UNKNOWN:
> +        /* could not be determined or s.th. is seriously wrong */
> +        return;
> +    case CTDIR_STATUS_CORRECTED:
> +        directionIn = !directionIn;
> +        break;
> +    case CTDIR_STATUS_OLD:
> +        break;
> +    }
> +
>      if (rule->tt != VIR_NWFILTER_RULE_DIRECTION_INOUT)
>          virBufferAsprintf(buf, " -m conntrack --ctdir %s",
>                            (directionIn) ? "Original"
> @@ -4304,6 +4327,34 @@ ebiptablesDriverTestCLITools(void)
>      return ret;
>  }
>  
> +static void
> +ebiptablesDriverProbeCtdir(void)
> +{
> +    struct utsname utsname;
> +    unsigned int major, minor, micro;
> +
> +    iptables_ctdir_corrected = CTDIR_STATUS_UNKNOWN;
> +
> +    if (uname(&utsname) < 0) {
> +        VIR_ERROR(_("Call to utsname failed: %d"), errno);
> +        return;
> +    }
> +
> +    /* following Linux lxr, the logic was inverted in 2.6.39 */
> +    if (sscanf(utsname.release, "%u.%u.%u", &major, &minor, &micro) != 3) {

Since these functions are only ever compiled on Linux, I think it's fine
that you're only checking release, and not looking at sysname (we
already know what it is.

I'm not a fan of sscanf(), and would prefer it was completely eliminated
from libvirt, but it is still in use in a few places, so I guess this
could go in too. But I would prefer something similar to the way we
parse the qemu version number at the top of virQEMUCapsParseHelpStr().

Does anyone else have an opinion one way or another on sscanf?

At any rate, ACK either way.


> +        VIR_ERROR(_("Could not detect kernel version from string %s"),
> +                  utsname.release);
> +        return;
> +    }
> +
> +    if (major >= 3 ||
> +        (major == 2 && minor == 6 && micro >= 39))
> +        iptables_ctdir_corrected = CTDIR_STATUS_CORRECTED;
> +    else
> +        iptables_ctdir_corrected = CTDIR_STATUS_OLD;
> +
> +}
> +
>  static int
>  ebiptablesDriverInit(bool privileged)
>  {
> @@ -4341,6 +4392,9 @@ ebiptablesDriverInit(bool privileged)
>          return -ENOTSUP;
>      }
>  
> +    if (iptables_cmd_path)
> +        ebiptablesDriverProbeCtdir();
> +
>      ebiptables_driver.flags = TECHDRV_FLAG_INITIALIZED;
>  
>      return 0;
>
>