[libvirt PATCH] network: add private chains only if there are networks adding iptables rules

Daniel Henrique Barboza danielhb413 at gmail.com
Mon Jun 8 18:39:06 UTC 2020



On 6/5/20 2:56 PM, Laine Stump wrote:
> Juan Quintela noticed that when he restarted libvirt he was getting
> extra iptables rules added by libvirt even though he didn't have any
> libvirt networks that used iptables rules. It turns out this also
> happens if the firewalld service is restarted. The extra rules are
> just the private chains, and they're sometimes being added
> unnecessarily because they are added separately in a global
> networkPreReloadFirewallRules() that does the init if there are any
> active networks, regardless of whether or not any of those networks
> will actually add rules to the host firewall.
> 
> The fix is to change the check for "any active networks" to instead
> check for "any active networks that add firewall rules".
> 
> (NB: although the timing seems suspicious, this isn't a new regression
> caused by the recently pushed f5418b427 (which forces recreation of
> private chains when firewalld is restarted); it was an existing bug
> since iptables rules were first put into private chains, even after
> commit c6cbe18771 delayed creation of the private chains. The
> implication is that any downstream based on v5.1.0 or later that cares
> about these extraneous (but harmless) private chains would want to
> backport this patch (along with the other two if they aren't already
> there))
> 
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
>   src/network/bridge_driver_linux.c | 49 ++++++++++++++++++++++++-------
>   1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index b0bd207250..4145411b4b 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void)
>   
>   
>   static int
> -networkHasRunningNetworksHelper(virNetworkObjPtr obj,
> +networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj,
>                                   void *opaque)
>   {
> -    bool *running = opaque;
> +    bool *activeWithFW = opaque;
>   
>       virObjectLock(obj);
> -    if (virNetworkObjIsActive(obj))
> -        *running = true;
> +    if (virNetworkObjIsActive(obj)) {
> +        virNetworkDefPtr def = virNetworkObjGetDef(obj);
> +
> +        switch ((virNetworkForwardType) def->forward.type) {
> +        case VIR_NETWORK_FORWARD_NONE:
> +        case VIR_NETWORK_FORWARD_NAT:
> +        case VIR_NETWORK_FORWARD_ROUTE:
> +            *activeWithFW = true;
> +            break;
> +


What's the rationale of "VIR_NETWORK_FORWARD_NONE" changing firewall rules? Is
this a corner case that the NONE type covers? Functions such as
networkAddIPSpecificFirewallRules() are operating just with the NAT and ROUTE
forward types.

(side note: there is no "firewall" string in formatdomain.html.in docs. I think
it's a good idea to mention that certain <forward> types will change firewall
settings of the host)



Thanks,


DHB





> +        case VIR_NETWORK_FORWARD_OPEN:
> +        case VIR_NETWORK_FORWARD_BRIDGE:
> +        case VIR_NETWORK_FORWARD_PRIVATE:
> +        case VIR_NETWORK_FORWARD_VEPA:
> +        case VIR_NETWORK_FORWARD_PASSTHROUGH:
> +        case VIR_NETWORK_FORWARD_HOSTDEV:
> +        case VIR_NETWORK_FORWARD_LAST:
> +            break;
> +        }
> +    }
> +
>       virObjectUnlock(obj);
>   
> +    /*
> +     * terminate ForEach early once we find an active network that
> +     * adds Firewall rules (return status is ignored)
> +     */
> +    if (*activeWithFW)
> +        return -1;
> +
>       return 0;
>   }
>   
>   
>   static bool
> -networkHasRunningNetworks(virNetworkDriverStatePtr driver)
> +networkHasRunningNetworksWithFW(virNetworkDriverStatePtr driver)
>   {
> -    bool running = false;
> +    bool activeWithFW = false;
> +
>       virNetworkObjListForEach(driver->networks,
> -                             networkHasRunningNetworksHelper,
> -                             &running);
> -    return running;
> +                             networkHasRunningNetworksWithFWHelper,
> +                             &activeWithFW);
> +    return activeWithFW;
>   }
>   
>   
> @@ -150,8 +177,8 @@ networkPreReloadFirewallRules(virNetworkDriverStatePtr driver,
>           networkSetupPrivateChains();
>   
>       } else {
> -        if (!networkHasRunningNetworks(driver)) {
> -            VIR_DEBUG("Delayed global rule setup as no networks are running");
> +        if (!networkHasRunningNetworksWithFW(driver)) {
> +            VIR_DEBUG("Delayed global rule setup as no networks with firewall rules are running");
>               return;
>           }
>   
> 




More information about the libvir-list mailing list