On 11/1/18 8:52 AM, Daniel P. Berrangé wrote:
> Historically rules were added straight into the base chains. This works
> but it is inflexible for admins adding extra rules via hook scripts, and
> it is not clear which rules are libvirt created.
>
> There is a further complexity with the FORWARD chain where a specific
> ordering of rules is needed to ensure traffic is matched correctly. This
> would require complex interleaving of rules instead of plain appending.
> By splitting the FORWARD chain into three chains management will be
> simpler. Thus we create
>
> INPUT -> INP_libvirt
> OUTPUT -> OUT_libvirt
> FORWARD -> FWD_libvirt_cross
> FORWARD -> FWD_libvirt_in
> FORWARD -> FWD_libvirt_out
> POSTROUTING -> PRT_libvirt
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 1 +
> src/util/viriptables.c | 81 ++++++++++++++++++++++++++++++++++++++++
> src/util/viriptables.h | 2 +
> 3 files changed, 84 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 335210c31d..e42c946de6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2062,6 +2062,7 @@ iptablesRemoveOutputFixUdpChecksum;
> iptablesRemoveTcpInput;
> iptablesRemoveUdpInput;
> iptablesRemoveUdpOutput;
> +iptablesSetupPrivateChains;
>
>
> # util/viriscsi.h
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index f379844d28..4a7ea54b38 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -51,6 +51,87 @@ enum {
> };
>
>
> +
> +typedef struct {
> + virFirewallLayer layer;
> + const char *table;
> + const char *parent;
> + const char *child;
> +} iptablesChain;
> +
> +static int
> +iptablesCheckPrivateChain(virFirewallPtr fw,
> + const char *const *lines,
> + void *opaque)
> +{
> + iptablesChain *data = opaque;
> + bool found = false;
> +
> + while (lines && *lines && !found) {
> + if (STRPREFIX(*lines, data->child))
> + found = true;
> + lines++;
> + }
> +
> + if (!found)
> + virFirewallAddRule(fw, data->layer,
> + "--table", data->table,
> + "--insert", data->parent,
> + "--jump", data->child, NULL);
> +
> + return 0;
> +}
> +
> +
> +int
> +iptablesSetupPrivateChains(void)
> +{
> + virFirewallPtr fw;
> + int ret = -1;
> + iptablesChain chains[] = {
> + {VIR_FIREWALL_LAYER_IPV4, "filter", "INPUT", "INP_libvirt"},
> + {VIR_FIREWALL_LAYER_IPV4, "filter", "OUTPUT", "OUT_libvirt"},
> + {VIR_FIREWALL_LAYER_IPV4, "filter", "FORWARD", "FWD_libvirt_out"},
> + {VIR_FIREWALL_LAYER_IPV4, "filter", "FORWARD", "FWD_libvirt_in"},
> + {VIR_FIREWALL_LAYER_IPV4, "filter", "FORWARD", "FWD_libvirt_cross"},
> + {VIR_FIREWALL_LAYER_IPV4, "nat", "POSTROUTING", "PRT_libvirt"},
> + {VIR_FIREWALL_LAYER_IPV6, "filter", "INPUT", "INP_libvirt"},
> + {VIR_FIREWALL_LAYER_IPV6, "filter", "OUTPUT", "OUT_libvirt"},
> + {VIR_FIREWALL_LAYER_IPV6, "filter", "FORWARD", "FWD_libvirt_out"},
> + {VIR_FIREWALL_LAYER_IPV6, "filter", "FORWARD", "FWD_libvirt_in"},
> + {VIR_FIREWALL_LAYER_IPV6, "filter", "FORWARD", "FWD_libvirt_cross"},
> + {VIR_FIREWALL_LAYER_IPV6, "nat", "POSTROUTING", "PRT_libvirt"},
> + };
> + size_t i;
> +
> + fw = virFirewallNew();
> +
> + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
> +
> + for (i = 0; i < ARRAY_CARDINALITY(chains); i++) {
> + virFirewallAddRule(fw, chains[i].layer,
> + "--table", chains[i].table,
> + "--new-chain", chains[i].child, NULL);
> + }
> +
> + virFirewallStartTransaction(fw, 0);
> +
> + for (i = 0; i < ARRAY_CARDINALITY(chains); i++) {
> + virFirewallAddRuleFull(fw, chains[i].layer,
> + false, iptablesCheckPrivateChain,
> + &chains[i],
> + "--table", chains[i].table,
> + "--list", chains[i].parent, NULL);
As we discussed on IRC last week, this *really* needs a "-n" to prevent
iptables from doing a DNS lookup on every IP address in every rule. On a
test I setup (with 60 networks) it took more than 10 minutes(!) to
restart libvirtd after upgrading to the new code. With the old code, a
restart after upgrading took 45 seconds.
Even after you do that, this still creates some slowdown, and a *lot* of
warnings in the logs from firewalld. A couple of ideas:
1) iptablesCheckPrivateChain only needs to be called once for each
combination of layer+table+child, but it's being called 3 times for
ipv4+filter+FORWARD and for ipv6+filter+FORWARD. Maybe the table could
be constructed differently so that there is one entry for each
layer+table+child, and each one of those entries has a list of all the
private chains needed.
2) The toplevel function is called for every new network, but really
only needs to be called a) when libvirtd is started, and b) when
firewalld notifies us that it has flushed all of the rules.
3) We only add the rule to jump to the new chain if that rule doesn't
exist already, but we still try to create the new chain no matter what,
leading to tons of firewalld warnings in the log about attempts to
create a new chain with the same name as an existing chain. The
existence of the "-j $chain" rule is a fairly reliable indicator that
the chain itself exists, though - we could eliminate these warnings (and
the extra unnecessary dbus call + iptables exec) if we would add the new
chain only in cases where we saw that the jump to the chain didn't exist.
> + }
> +
> + if (virFirewallApply(fw) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> static void
> iptablesInput(virFirewallPtr fw,
> virFirewallLayer layer,
> diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> index 9ea25fc096..1db97937a1 100644
> --- a/src/util/viriptables.h
> +++ b/src/util/viriptables.h
> @@ -27,6 +27,8 @@
> # include "virsocketaddr.h"
> # include "virfirewall.h"
>
> +int iptablesSetupPrivateChains (void);
> +
> void iptablesAddTcpInput (virFirewallPtr fw,
> virFirewallLayer layer,
> const char *iface,
Attachment:
pEpkey.asc
Description: application/pgp-keys
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
