zebra: avoid pbr iptable added twice when used with flowspec

The usage of zebra dplane makes the job asyncronous which implies
that a given job will try to add an iptable, while the second job
will not know that its iptable is the same as the former one.

The below exabgp rules stand for two bgp flowspec rules sent to
the bgp device:

flow {
route {match {
source 185.228.172.73/32;
destination 0.0.0.0/0;
source-port >=49156&<=49159;
}then {redirect 213.242.114.113;}}
route {match {
source 185.228.172.73/32;
destination 0.0.0.0/0;
source-port >=49160&<=49163;
}then {redirect 213.242.114.113;}}
}

This rule creates a single iptable, but in fact, the same iptable
name is appended twice. This results in duplicated entries in the
iptables context. This also results in contexts not flushed, when
BGP session or 'flush' operation is performed.

iptables-save:
[..]
-A PREROUTING -m set --match-set match0x55baf4c25cb0 src,src -g match0x55baf4c25cb0
-A PREROUTING -m set --match-set match0x55baf4c25cb0 src,src -g match0x55baf4c25cb0
-A match0x55baf4c25cb0 -j MARK --set-xmark 0x100/0xffffffff
-A match0x55baf4c25cb0 -j ACCEPT
-A match0x55baf4c25cb0 -j MARK --set-xmark 0x100/0xffffffff
-A match0x55baf4c25cb0 -j ACCEPT
[..]

This commit addresses this issue, by checking that an iptable
context is not already being processed. A flag is added in the
original iptable context, and a check is done if the iptable
context is not already being processed for install or uinstall.

Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c
index 408d420be6ffff13a923482c3b4ffd1e051d4e4d..9a30c2b78facc3666c28a4627fe6d59a7b8353dc 100644 (file)
--- a/zebra/zapi_msg.c
+++ b/zebra/zapi_msg.c
@@ -876,9 +876,24 @@ void zsend_iptable_notify_owner(const struct zebra_dplane_ctx *ctx,
        struct stream *s;
        struct zebra_pbr_iptable ipt;
        uint16_t cmd = ZEBRA_IPTABLE_NOTIFY_OWNER;
+       struct zebra_pbr_iptable *ipt_hash;
+       enum dplane_op_e op = dplane_ctx_get_op(ctx);
 
        dplane_ctx_get_pbr_iptable(ctx, &ipt);
 
+       ipt_hash = hash_lookup(zrouter.iptable_hash, &ipt);
+       if (ipt_hash) {
+               if (op == DPLANE_OP_IPTABLE_ADD &&
+                   CHECK_FLAG(ipt_hash->internal_flags,
+                              IPTABLE_INSTALL_QUEUED))
+                       UNSET_FLAG(ipt_hash->internal_flags,
+                                  IPTABLE_INSTALL_QUEUED);
+               else if (op == DPLANE_OP_IPTABLE_DELETE &&
+                        CHECK_FLAG(ipt_hash->internal_flags,
+                                   IPTABLE_UNINSTALL_QUEUED))
+                       UNSET_FLAG(ipt_hash->internal_flags,
+                                  IPTABLE_UNINSTALL_QUEUED);
+       }
        if (IS_ZEBRA_DEBUG_PACKET)
                zlog_debug("%s: Notifying %s id %u note %u", __func__,
                           zserv_command_string(cmd), ipt.unique, note);

diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c
index 4e753c9d1a0312f5b606ff4b014fabee76b0debf..0da44e3c4ee90b3bcd691eafdab124599fdef62b 100644 (file)
--- a/zebra/zebra_dplane.c
+++ b/zebra/zebra_dplane.c
@@ -4550,6 +4550,17 @@ iptable_update_internal(enum dplane_op_e op, struct zebra_pbr_iptable *iptable)
        struct zebra_dplane_ctx *ctx;
        int ret;
 
+       if ((op == DPLANE_OP_IPTABLE_ADD &&
+            CHECK_FLAG(iptable->internal_flags, IPTABLE_INSTALL_QUEUED)) ||
+           (op == DPLANE_OP_IPTABLE_DELETE &&
+            CHECK_FLAG(iptable->internal_flags, IPTABLE_UNINSTALL_QUEUED))) {
+               if (IS_ZEBRA_DEBUG_DPLANE_DETAIL)
+                       zlog_debug(
+                               "update dplane ctx %s: iptable %s already in progress",
+                               dplane_op2str(op), iptable->ipset_name);
+               return result;
+       }
+
        ctx = dplane_ctx_alloc();
 
        ret = dplane_ctx_iptable_init(ctx, op, iptable);
@@ -4562,14 +4573,19 @@ done:
        atomic_fetch_add_explicit(&zdplane_info.dg_iptable_in, 1,
                                  memory_order_relaxed);
 
-       if (ret == AOK)
+       if (ret == AOK) {
                result = ZEBRA_DPLANE_REQUEST_QUEUED;
-       else {
+               if (op == DPLANE_OP_IPTABLE_ADD)
+                       SET_FLAG(iptable->internal_flags,
+                                IPTABLE_INSTALL_QUEUED);
+               else
+                       SET_FLAG(iptable->internal_flags,
+                                IPTABLE_UNINSTALL_QUEUED);
+       } else {
                atomic_fetch_add_explicit(&zdplane_info.dg_iptable_errors, 1,
                                          memory_order_relaxed);
                dplane_ctx_free(&ctx);
        }
-
        return result;
 }
 

diff --git a/zebra/zebra_pbr.c b/zebra/zebra_pbr.c
index e66d7aaf5c8aab9fe1c405db9d9006fa73d07698..bb963bcc233e7fd01fc6c53941f210b13f9cccf1 100644 (file)
--- a/zebra/zebra_pbr.c
+++ b/zebra/zebra_pbr.c
@@ -812,8 +812,11 @@ static void *pbr_iptable_alloc_intern(void *arg)
 
 void zebra_pbr_add_iptable(struct zebra_pbr_iptable *iptable)
 {
-       (void)hash_get(zrouter.iptable_hash, iptable, pbr_iptable_alloc_intern);
-       (void)dplane_pbr_iptable_add(iptable);
+       struct zebra_pbr_iptable *ipt_hash;
+
+       ipt_hash = hash_get(zrouter.iptable_hash, iptable,
+                           pbr_iptable_alloc_intern);
+       (void)dplane_pbr_iptable_add(ipt_hash);
 }
 
 void zebra_pbr_del_iptable(struct zebra_pbr_iptable *iptable)

diff --git a/zebra/zebra_pbr.h b/zebra/zebra_pbr.h
index c5102df4fa8898fec6fa79c1bbdd05d007aa76f5..33b8162a8867ee851363900f7aa175c984f24b89 100644 (file)
--- a/zebra/zebra_pbr.h
+++ b/zebra/zebra_pbr.h
@@ -171,6 +171,9 @@ struct zebra_pbr_iptable {
 
        struct list *interface_name_list;
 
+#define IPTABLE_INSTALL_QUEUED 1 << 1
+#define IPTABLE_UNINSTALL_QUEUED 1 << 2
+       uint8_t internal_flags;
        char ipset_name[ZEBRA_IPSET_NAME_SIZE];
 };