[libvirt] [PATCH] [nwfilter] Fix libvirt upgrade path when nwfilter is used

[Laine Stump]

On 02/14/2013 06:26 PM, Stefan Berger wrote:
> Between revision 65fb9d49 and before this patch, an upgrade of libvirt
> while
> VMs are running and instantiating iptables filtering rules due to
> nwfilter
> rules, may leave stray iptables rules behind when shutting VMs down.
> Left-over iptables rules may look like this:
>
> Chain FP-vnet0 (1 references)
> target     prot opt source               destination
> DROP       tcp  --  0.0.0.0/0            0.0.0.0/0            tcp spt:122
> ACCEPT     all  --  0.0.0.0/0            0.0.0.0/0
>
> [...]
>
> Chain libvirt-out (1 references)
> target     prot opt source               destination
> FO-vnet0   all  --  0.0.0.0/0            0.0.0.0/0 [goto]  PHYSDEV
> match --physdev-out vnet0
>
>
>
> The reason is that the recent nwfilter code only removed filtering
> rules in
> the libvirt-out chain that contain the --physdev-is-bridged parameter.
> Older rules didn't match and were not removed.
>
> Note that the user-defined chain FO-vnet0 could not be removed due to the
> reference from the rule in libvirt-out.
>
> Often the work around may be done through
>
> service iptables restart
> kill -SIGHUP $(pidof libvirtd)
>
> This patch now also removes older libvirt versions' iptables rules.
>
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>

Assuming that the scripts you create ignore non-zero exit codes and
extra output to stdout (which would both bethe result of running theses
new commands on systems that don't need them), ACK.

(Actually that is a problem for the network driver's iptables usage - if
it tries to remove a rule that isn't there, a warning will be put in the
system log.)

>
> ---
>  src/nwfilter/nwfilter_ebiptables_driver.c |   22 ++++++++++++++++++++++
>  1 file changed, 22 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
> @@ -167,16 +167,24 @@ static const char ebiptables_script_set_
>
>  #define PHYSDEV_IN  "--physdev-in"
>  #define PHYSDEV_OUT "--physdev-is-bridged --physdev-out"
> +/*
> + * Previous versions of libvirt only used --physdev-out.
> + * To be able to upgrade with running VMs we need to be able to
> + * remove rules generated by those older versions of libvirt.
> + */
> +#define PHYSDEV_OUT_OLD  "--physdev-out"
>
>  static const char *m_state_out_str   = "-m state --state
> NEW,ESTABLISHED";
>  static const char *m_state_in_str    = "-m state --state ESTABLISHED";
>  static const char *m_physdev_in_str  = "-m physdev " PHYSDEV_IN;
>  static const char *m_physdev_out_str = "-m physdev " PHYSDEV_OUT;
> +static const char *m_physdev_out_old_str = "-m physdev "
> PHYSDEV_OUT_OLD;
>
>  #define MATCH_STATE_OUT    m_state_out_str
>  #define MATCH_STATE_IN     m_state_in_str
>  #define MATCH_PHYSDEV_IN   m_physdev_in_str
>  #define MATCH_PHYSDEV_OUT  m_physdev_out_str
> +#define MATCH_PHYSDEV_OUT_OLD  m_physdev_out_old_str
>
>  #define COMMENT_VARNAME "comment"
>
> @@ -821,6 +829,8 @@ _iptablesUnlinkRootChain(virBufferPtr bu
>                                      : CHAINPREFIX_HOST_OUT;
>      const char *match = (incoming) ? MATCH_PHYSDEV_IN
>                                     : MATCH_PHYSDEV_OUT;
> +    const char *old_match = (incoming) ? NULL
> +                                       : MATCH_PHYSDEV_OUT_OLD;
>
>      PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname);
>
> @@ -830,6 +840,18 @@ _iptablesUnlinkRootChain(virBufferPtr bu
>                        basechain,
>                        match, ifname, chain);
>
> +    /*
> +     * Previous versions of libvirt may have created a rule
> +     * with the --physdev-is-bridged missing. Remove this one
> +     * as well.
> +     */
> +    if (old_match)
> +        virBufferAsprintf(buf,
> +                          "$IPT -D %s "
> +                          "%s %s -g %s" CMD_SEPARATOR,
> +                          basechain,
> +                          old_match, ifname, chain);
> +
>      return 0;
>  }
>
>