child-sa: Replace reqid based marks by "unique" marks

As we now use the same reqid for multiple CHILD_SAs with the same selectors,
having marks based on the reqid makes not that much sense anymore. Instead we
use unique marks that use a custom identifier. This identifier is reused during
rekeying, keeping the marks constant for any rule relying on it (for example
installed by updown).

This also simplifies handling of reqid allocation, as we do not have to query
the marks that is not yet assigned for an unknown reqid.

diff --git a/src/libcharon/plugins/ha/ha_dispatcher.c b/src/libcharon/plugins/ha/ha_dispatcher.c
index e20e872c181fd4a55d46382aa9b9ed2153fa5b4a..6e02733f9e686826e6f01e9d645733538a7e3a8b 100644 (file)
--- a/src/libcharon/plugins/ha/ha_dispatcher.c
+++ b/src/libcharon/plugins/ha/ha_dispatcher.c
@@ -718,7 +718,8 @@ static void process_child_add(private_ha_dispatcher_t *this,
 
        child_sa = child_sa_create(ike_sa->get_my_host(ike_sa),
                                                           ike_sa->get_other_host(ike_sa), config, 0,
-                                                          ike_sa->has_condition(ike_sa, COND_NAT_ANY));
+                                                          ike_sa->has_condition(ike_sa, COND_NAT_ANY),
+                                                          0, 0);
        child_sa->set_mode(child_sa, mode);
        child_sa->set_protocol(child_sa, PROTO_ESP);
        child_sa->set_ipcomp(child_sa, ipcomp);

diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c
index 7625be1d602e6b5b472c0e730045c6b4629d6f42..fdeb605eeeb839b2b8bd6c49f84fea65d516eccb 100644 (file)
--- a/src/libcharon/sa/child_sa.c
+++ b/src/libcharon/sa/child_sa.c
@@ -695,7 +695,7 @@ METHOD(child_sa_t, install, status_t,
        if (!this->reqid_allocated)
        {
                status = hydra->kernel_interface->alloc_reqid(hydra->kernel_interface,
-                                                       my_ts, other_ts, &this->mark_in, &this->mark_out,
+                                                       my_ts, other_ts, this->mark_in, this->mark_out,
                                                        &this->reqid);
                if (status != SUCCESS)
                {
@@ -825,7 +825,7 @@ METHOD(child_sa_t, add_policies, status_t,
                /* trap policy, get or confirm reqid */
                status = hydra->kernel_interface->alloc_reqid(
                                                        hydra->kernel_interface, my_ts_list, other_ts_list,
-                                                       &this->mark_in, &this->mark_out, &this->reqid);
+                                                       this->mark_in, this->mark_out, &this->reqid);
                if (status != SUCCESS)
                {
                        return status;
@@ -1198,10 +1198,11 @@ static host_t* get_proxy_addr(child_cfg_t *config, host_t *ike, bool local)
  * Described in header.
  */
 child_sa_t * child_sa_create(host_t *me, host_t* other,
-                                                        child_cfg_t *config, u_int32_t rekey, bool encap)
+                                                        child_cfg_t *config, u_int32_t rekey, bool encap,
+                                                        u_int mark_in, u_int mark_out)
 {
        private_child_sa_t *this;
-       static refcount_t unique_id = 0;
+       static refcount_t unique_id = 0, unique_mark = 0, mark;
 
        INIT(this,
                .public = {
@@ -1258,6 +1259,28 @@ child_sa_t * child_sa_create(host_t *me, host_t* other,
        this->config = config;
        config->get_ref(config);
 
+       if (mark_in)
+       {
+               this->mark_in.value = mark_in;
+       }
+       if (mark_out)
+       {
+               this->mark_out.value = mark_out;
+       }
+       if (this->mark_in.value == MARK_UNIQUE ||
+               this->mark_out.value == MARK_UNIQUE)
+       {
+               mark = ref_get(&unique_mark);
+               if (this->mark_in.value == MARK_UNIQUE)
+               {
+                       this->mark_in.value = mark;
+               }
+               if (this->mark_out.value == MARK_UNIQUE)
+               {
+                       this->mark_out.value = mark;
+               }
+       }
+
        if (!this->reqid)
        {
                /* reuse old reqid if we are rekeying an existing CHILD_SA. While the

diff --git a/src/libcharon/sa/child_sa.h b/src/libcharon/sa/child_sa.h
index f0ec0165824598bbdc81f2ac8de38a04b5d4f6be..83b8c096cdbf7e080166fb460c022418a3a4072c 100644 (file)
--- a/src/libcharon/sa/child_sa.h
+++ b/src/libcharon/sa/child_sa.h
@@ -394,9 +394,12 @@ struct child_sa_t {
  * @param config                       config to use for this CHILD_SA
  * @param reqid                                reqid of old CHILD_SA when rekeying, 0 otherwise
  * @param encap                                TRUE to enable UDP encapsulation (NAT traversal)
+ * @param mark_in                      explicit inbound mark value to use, 0 for config
+ * @param mark_out                     explicit outbound mark value to use, 0 for config
  * @return                                     child_sa_t object
  */
 child_sa_t * child_sa_create(host_t *me, host_t *other, child_cfg_t *config,
-                                                        u_int32_t reqid, bool encap);
+                                                        u_int32_t reqid, bool encap,
+                                                        u_int mark_in, u_int mark_out);
 
 #endif /** CHILD_SA_H_ @}*/

diff --git a/src/libcharon/sa/ikev1/task_manager_v1.c b/src/libcharon/sa/ikev1/task_manager_v1.c
index 0f8e8bc6dd5e18efdea4bdf1ce246e2131d1dbf7..3184db4eec7c1ce7b73c60f0d21b5268a393307d 100644 (file)
--- a/src/libcharon/sa/ikev1/task_manager_v1.c
+++ b/src/libcharon/sa/ikev1/task_manager_v1.c
@@ -1647,6 +1647,8 @@ METHOD(task_manager_t, queue_child_rekey, void,
                        task = quick_mode_create(this->ike_sa, cfg->get_ref(cfg),
                                get_first_ts(child_sa, TRUE), get_first_ts(child_sa, FALSE));
                        task->use_reqid(task, child_sa->get_reqid(child_sa));
+                       task->use_marks(task, child_sa->get_mark(child_sa, TRUE).value,
+                                                       child_sa->get_mark(child_sa, FALSE).value);
                        task->rekey(task, child_sa->get_spi(child_sa, TRUE));
 
                        queue_task(this, &task->task);

diff --git a/src/libcharon/sa/ikev1/tasks/quick_mode.c b/src/libcharon/sa/ikev1/tasks/quick_mode.c
index 1133aab65b63f479ace48b7b77717440091d4f86..5fe04c03615087d52005feee67c27a6c4174083d 100644 (file)
--- a/src/libcharon/sa/ikev1/tasks/quick_mode.c
+++ b/src/libcharon/sa/ikev1/tasks/quick_mode.c
@@ -155,6 +155,16 @@ struct private_quick_mode_t {
         */
        u_int32_t reqid;
 
+       /**
+        * Explicit inbound mark value to use, if any
+        */
+       u_int mark_in;
+
+       /**
+        * Explicit inbound mark value to use, if any
+        */
+       u_int mark_out;
+
        /**
         * SPI of SA we rekey
         */
@@ -788,7 +798,8 @@ METHOD(task_t, build_i, status_t,
                        this->child_sa = child_sa_create(
                                                                        this->ike_sa->get_my_host(this->ike_sa),
                                                                        this->ike_sa->get_other_host(this->ike_sa),
-                                                                       this->config, this->reqid, this->udp);
+                                                                       this->config, this->reqid, this->udp,
+                                                                       this->mark_in, this->mark_out);
 
                        if (this->udp && this->mode == MODE_TRANSPORT)
                        {
@@ -972,6 +983,10 @@ static void check_for_rekeyed_child(private_quick_mode_t *this)
                                        {
                                                this->reqid = child_sa->get_reqid(child_sa);
                                                this->rekey = child_sa->get_spi(child_sa, TRUE);
+                                               this->mark_in = child_sa->get_mark(child_sa,
+                                                                                                                       TRUE).value;
+                                               this->mark_out = child_sa->get_mark(child_sa,
+                                                                                                                       FALSE).value;
                                                child_sa->set_state(child_sa, CHILD_REKEYING);
                                                DBG1(DBG_IKE, "detected rekeying of CHILD_SA %s{%u}",
                                                         child_sa->get_name(child_sa), this->reqid);
@@ -1097,7 +1112,8 @@ METHOD(task_t, process_r, status_t,
                        this->child_sa = child_sa_create(
                                                                        this->ike_sa->get_my_host(this->ike_sa),
                                                                        this->ike_sa->get_other_host(this->ike_sa),
-                                                                       this->config, this->reqid, this->udp);
+                                                                       this->config, this->reqid, this->udp,
+                                                                       this->mark_in, this->mark_out);
 
                        tsi = linked_list_create_with_items(this->tsi, NULL);
                        tsr = linked_list_create_with_items(this->tsr, NULL);
@@ -1307,6 +1323,13 @@ METHOD(quick_mode_t, use_reqid, void,
        this->reqid = reqid;
 }
 
+METHOD(quick_mode_t, use_marks, void,
+       private_quick_mode_t *this, u_int in, u_int out)
+{
+       this->mark_in = in;
+       this->mark_out = out;
+}
+
 METHOD(quick_mode_t, rekey, void,
        private_quick_mode_t *this, u_int32_t spi)
 {
@@ -1334,6 +1357,8 @@ METHOD(task_t, migrate, void,
        this->dh = NULL;
        this->spi_i = 0;
        this->spi_r = 0;
+       this->mark_in = 0;
+       this->mark_out = 0;
 
        if (!this->initiator)
        {
@@ -1372,6 +1397,7 @@ quick_mode_t *quick_mode_create(ike_sa_t *ike_sa, child_cfg_t *config,
                                .destroy = _destroy,
                        },
                        .use_reqid = _use_reqid,
+                       .use_marks = _use_marks,
                        .rekey = _rekey,
                },
                .ike_sa = ike_sa,

diff --git a/src/libcharon/sa/ikev1/tasks/quick_mode.h b/src/libcharon/sa/ikev1/tasks/quick_mode.h
index 0b80cb836e49e5ea5294c1ce56b9aa3aca57f132..ee9b64d1394cdde4f6024e451565c434164e5e3b 100644 (file)
--- a/src/libcharon/sa/ikev1/tasks/quick_mode.h
+++ b/src/libcharon/sa/ikev1/tasks/quick_mode.h
@@ -44,6 +44,14 @@ struct quick_mode_t {
         */
        void (*use_reqid)(quick_mode_t *this, u_int32_t reqid);
 
+       /**
+        * Use specific mark values, overriding configuration.
+        *
+        * @param in                    inbound mark value
+        * @param out                   outbound mark value
+        */
+       void (*use_marks)(quick_mode_t *this, u_int in, u_int out);
+
        /**
         * Set the SPI of the old SA, if rekeying.
         *

diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c
index e7a9148757fdaf7364b555b6ce7374702c1931b1..5ec05376c0f494fdb100f84d92f6a577438e4880 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_create.c
+++ b/src/libcharon/sa/ikev2/tasks/child_create.c
@@ -159,6 +159,16 @@ struct private_child_create_t {
         */
        u_int32_t reqid;
 
+       /**
+        * Explicit inbound mark value
+        */
+       u_int mark_in;
+
+       /**
+        * Explicit outbound mark value
+        */
+       u_int mark_out;
+
        /**
         * CHILD_SA which gets established
         */
@@ -996,7 +1006,8 @@ METHOD(task_t, build_i, status_t,
 
        this->child_sa = child_sa_create(this->ike_sa->get_my_host(this->ike_sa),
                        this->ike_sa->get_other_host(this->ike_sa), this->config, this->reqid,
-                       this->ike_sa->has_condition(this->ike_sa, COND_NAT_ANY));
+                       this->ike_sa->has_condition(this->ike_sa, COND_NAT_ANY),
+                       this->mark_in, this->mark_out);
 
        if (!allocate_spi(this))
        {
@@ -1241,7 +1252,8 @@ METHOD(task_t, build_r, status_t,
 
        this->child_sa = child_sa_create(this->ike_sa->get_my_host(this->ike_sa),
                        this->ike_sa->get_other_host(this->ike_sa), this->config, this->reqid,
-                       this->ike_sa->has_condition(this->ike_sa, COND_NAT_ANY));
+                       this->ike_sa->has_condition(this->ike_sa, COND_NAT_ANY),
+                       this->mark_in, this->mark_out);
 
        if (this->ipcomp_received != IPCOMP_NONE)
        {
@@ -1478,6 +1490,13 @@ METHOD(child_create_t, use_reqid, void,
        this->reqid = reqid;
 }
 
+METHOD(child_create_t, use_marks, void,
+       private_child_create_t *this, u_int in, u_int out)
+{
+       this->mark_in = in;
+       this->mark_out = out;
+}
+
 METHOD(child_create_t, get_child, child_sa_t*,
        private_child_create_t *this)
 {
@@ -1545,6 +1564,8 @@ METHOD(task_t, migrate, void,
        this->ipcomp_received = IPCOMP_NONE;
        this->other_cpi = 0;
        this->reqid = 0;
+       this->mark_in = 0;
+       this->mark_out = 0;
        this->established = FALSE;
 }
 
@@ -1593,6 +1614,7 @@ child_create_t *child_create_create(ike_sa_t *ike_sa,
                        .set_config = _set_config,
                        .get_lower_nonce = _get_lower_nonce,
                        .use_reqid = _use_reqid,
+                       .use_marks = _use_marks,
                        .task = {
                                .get_type = _get_type,
                                .migrate = _migrate,

diff --git a/src/libcharon/sa/ikev2/tasks/child_create.h b/src/libcharon/sa/ikev2/tasks/child_create.h
index d29ba3d987bc410a4f0f6f5d07063000297ce7c2..46d9403eec3cb9a583421cbce6a077bb2baa7d43 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_create.h
+++ b/src/libcharon/sa/ikev2/tasks/child_create.h
@@ -51,6 +51,14 @@ struct child_create_t {
         */
        void (*use_reqid) (child_create_t *this, u_int32_t reqid);
 
+       /**
+        * Use specific mark values to override configuration.
+        *
+        * @param in            inbound mark value
+        * @param out           outbound mark value
+        */
+       void (*use_marks)(child_create_t *this, u_int in, u_int out);
+
        /**
         * Get the lower of the two nonces, used for rekey collisions.
         *

diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c
index db872827d78084fb9bb0e0671681fdad20829f46..213155a294e1f6bb9bdf1f8045c43cdb066f5158 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_rekey.c
+++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c
@@ -184,6 +184,9 @@ METHOD(task_t, build_i, status_t,
        }
        reqid = this->child_sa->get_reqid(this->child_sa);
        this->child_create->use_reqid(this->child_create, reqid);
+       this->child_create->use_marks(this->child_create,
+                                               this->child_sa->get_mark(this->child_sa, TRUE).value,
+                                               this->child_sa->get_mark(this->child_sa, FALSE).value);
 
        if (this->child_create->task.build(&this->child_create->task,
                                                                           message) != NEED_MORE)
@@ -224,6 +227,9 @@ METHOD(task_t, build_r, status_t,
        /* let the CHILD_CREATE task build the response */
        reqid = this->child_sa->get_reqid(this->child_sa);
        this->child_create->use_reqid(this->child_create, reqid);
+       this->child_create->use_marks(this->child_create,
+                                               this->child_sa->get_mark(this->child_sa, TRUE).value,
+                                               this->child_sa->get_mark(this->child_sa, FALSE).value);
        config = this->child_sa->get_config(this->child_sa);
        this->child_create->set_config(this->child_create, config->get_ref(config));
        this->child_create->task.build(&this->child_create->task, message);

diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index 7e55d6b0f5b032a36ed81960645174ece775ca42..534d4d5ff97e7477e5e8810c833e8d2ef14dd247 100644 (file)
--- a/src/libcharon/sa/trap_manager.c
+++ b/src/libcharon/sa/trap_manager.c
@@ -171,7 +171,7 @@ METHOD(trap_manager_t, install, u_int32_t,
        this->lock->unlock(this->lock);
 
        /* create and route CHILD_SA */
-       child_sa = child_sa_create(me, other, child, reqid, FALSE);
+       child_sa = child_sa_create(me, other, child, reqid, FALSE, 0, 0);
 
        list = linked_list_create_with_items(me, NULL);
        my_ts = child->get_traffic_selectors(child, TRUE, NULL, list);

diff --git a/src/libhydra/kernel/kernel_interface.c b/src/libhydra/kernel/kernel_interface.c
index 9452b8f847a64eef8aeec566097a9f8d9dd985f5..28821fc15e726f937e09e56bf47cbc5ca2a02e61 100644 (file)
--- a/src/libhydra/kernel/kernel_interface.c
+++ b/src/libhydra/kernel/kernel_interface.c
@@ -260,7 +260,9 @@ static u_int hash_ts_array(array_t *array, u_int hash)
  */
 static u_int hash_reqid_by_ts(reqid_entry_t *entry)
 {
-       return hash_ts_array(entry->local, hash_ts_array(entry->remote, 0));
+       return hash_ts_array(entry->local, hash_ts_array(entry->remote,
+                       chunk_hash_inc(chunk_from_thing(entry->mark_in),
+                               chunk_hash(chunk_from_thing(entry->mark_out)))));
 }
 
 /**
@@ -289,44 +291,17 @@ static bool ts_array_equals(array_t *a, array_t *b)
        return equal;
 }
 
-/**
- * Check if mark b matches to a, optionally with reqid match
- */
-static bool mark_matches(mark_t a, mark_t b, u_int32_t reqid)
-{
-       if (a.value == b.value)
-       {
-               return TRUE;
-       }
-       if (a.value == MARK_REQID && b.value == reqid)
-       {
-               return TRUE;
-       }
-       return FALSE;
-}
-
 /**
  * Hashtable equals function for reqid entries using traffic selectors as key
  */
 static bool equals_reqid_by_ts(reqid_entry_t *a, reqid_entry_t *b)
 {
-       if (ts_array_equals(a->local, b->local) &&
-               ts_array_equals(a->remote, b->remote) &&
-               a->mark_in.mask == b->mark_in.mask &&
-               a->mark_out.mask == b->mark_out.mask)
-       {
-               if (mark_matches(a->mark_in, b->mark_in, a->reqid) &&
-                       mark_matches(a->mark_out, b->mark_out, a->reqid))
-               {
-                       return TRUE;
-               }
-               if (mark_matches(b->mark_in, a->mark_in, b->reqid) &&
-                       mark_matches(b->mark_out, a->mark_out, b->reqid))
-               {
-                       return TRUE;
-               }
-       }
-       return FALSE;
+       return ts_array_equals(a->local, b->local) &&
+                  ts_array_equals(a->remote, b->remote) &&
+                  a->mark_in.value == b->mark_in.value &&
+                  a->mark_in.mask == b->mark_in.mask &&
+                  a->mark_out.value == b->mark_out.value &&
+                  a->mark_out.mask == b->mark_out.mask;
 }
 
 /**
@@ -353,7 +328,7 @@ static array_t *array_from_ts_list(linked_list_t *list)
 METHOD(kernel_interface_t, alloc_reqid, status_t,
        private_kernel_interface_t *this,
        linked_list_t *local_ts, linked_list_t *remote_ts,
-       mark_t *mark_in, mark_t *mark_out, u_int32_t *reqid)
+       mark_t mark_in, mark_t mark_out, u_int32_t *reqid)
 {
        static u_int32_t counter = 0;
        reqid_entry_t *entry = NULL, *tmpl;
@@ -362,8 +337,8 @@ METHOD(kernel_interface_t, alloc_reqid, status_t,
        INIT(tmpl,
                .local = array_from_ts_list(local_ts),
                .remote = array_from_ts_list(remote_ts),
-               .mark_in = *mark_in,
-               .mark_out = *mark_out,
+               .mark_in = mark_in,
+               .mark_out = mark_out,
                .reqid = *reqid,
        );
 
@@ -371,14 +346,6 @@ METHOD(kernel_interface_t, alloc_reqid, status_t,
        if (tmpl->reqid)
        {
                /* search by reqid if given */
-               if (tmpl->mark_in.value == MARK_REQID)
-               {
-                       tmpl->mark_in.value = tmpl->reqid;
-               }
-               if (tmpl->mark_out.value == MARK_REQID)
-               {
-                       tmpl->mark_out.value = tmpl->reqid;
-               }
                entry = this->reqids->get(this->reqids, tmpl);
        }
        if (entry)
@@ -390,8 +357,7 @@ METHOD(kernel_interface_t, alloc_reqid, status_t,
        }
        else
        {
-               /* search by traffic selectors. We do the search with MARK_REQID
-                * wildcards (if any), and update the marks if we find any match */
+               /* search by traffic selectors */
                entry = this->reqids_by_ts->get(this->reqids_by_ts, tmpl);
                if (entry)
                {
@@ -402,21 +368,11 @@ METHOD(kernel_interface_t, alloc_reqid, status_t,
                        /* none found, create a new entry, allocating a reqid */
                        entry = tmpl;
                        entry->reqid = ++counter;
-                       if (entry->mark_in.value == MARK_REQID)
-                       {
-                               entry->mark_in.value = entry->reqid;
-                       }
-                       if (entry->mark_out.value == MARK_REQID)
-                       {
-                               entry->mark_out.value = entry->reqid;
-                       }
                        this->reqids_by_ts->put(this->reqids_by_ts, entry, entry);
                        this->reqids->put(this->reqids, entry, entry);
                }
                *reqid = entry->reqid;
        }
-       *mark_in = entry->mark_in;
-       *mark_out = entry->mark_out;
        entry->refs++;
        this->mutex->unlock(this->mutex);
 

diff --git a/src/libhydra/kernel/kernel_interface.h b/src/libhydra/kernel/kernel_interface.h
index f25c1083085869367b7f1399fc4c9d836809f2cb..9a86e78d613e293f22694a1401121d72c22bd640 100644 (file)
--- a/src/libhydra/kernel/kernel_interface.h
+++ b/src/libhydra/kernel/kernel_interface.h
@@ -131,9 +131,6 @@ struct kernel_interface_t {
         * the reqid is confirmed and registered for use. If it points to zero,
         * a reqid is allocated for the given selectors, and returned to reqid.
         *
-        * The passed mark values get updated to the reqid value if they are set
-        * to the magic value MARK_REQID.
-        *
         * @param local_ts      traffic selectors of local side for SA
         * @param remote_ts     traffic selectors of remote side for SA
         * @param mark_in       inbound mark on SA
@@ -143,7 +140,7 @@ struct kernel_interface_t {
         */
        status_t (*alloc_reqid)(kernel_interface_t *this,
                                                        linked_list_t *local_ts, linked_list_t *remote_ts,
-                                                       mark_t *mark_in, mark_t *mark_out,
+                                                       mark_t mark_in, mark_t mark_out,
                                                        u_int32_t *reqid);
 
        /**

diff --git a/src/libstrongswan/ipsec/ipsec_types.h b/src/libstrongswan/ipsec/ipsec_types.h
index c1465e097f6eeeb23955a1593fac68e14aa28e1f..fa122af304d1e3081ad738df6e4d1a04289d3985 100644 (file)
--- a/src/libstrongswan/ipsec/ipsec_types.h
+++ b/src/libstrongswan/ipsec/ipsec_types.h
@@ -169,9 +169,9 @@ struct mark_t {
 };
 
 /**
- * Special mark value that uses the reqid of the CHILD_SA as mark
+ * Special mark value that uses a unique mark for each CHILD_SA
  */
-#define MARK_REQID (0xFFFFFFFF)
+#define MARK_UNIQUE (0xFFFFFFFF)
 
 /**
  * Try to parse a mark_t from the given string of the form mark[/mask].