> Our send/receive_delay testing hooks are insufficient to simulate this,
> I'll have to add some conditional packet delays to reproduce it.
I have added a simple conditional packet delay mechanism, it allows us
to delay some specific packets and reproduce this issue.
> I'll try to reproduce this condition
I configured box with:
strongswan.conf:
send_delay = 1000
receive_delay = 5000
receive_delay_type = 36
receive_delay_request = no
ipsec.conf:
lifetime=30s
margintime=11s
rekeyfuzz=0%
For xob, I used:
strongswan.conf:
send_delay = 1000
ipsec.conf:
lifetime=30s
margintime=10s
rekeyfuzz=0%
This will trigger a simultaneous rekey in ~20s, but will delay the
problematic CREATE_CHILD_SA response a little more. With this
configuration, I could reproduce the problem.
> I think it should be possible to catch this in the collide() check.
I additionally checked in a patch that fixes the problem for this
configuration for both cases, if xob wins the nonce compare, but also if
it loses.
Patches attached, please verify.
Best regards
Martin
>From 45def2147be9b6443bab69fe5457e3cf87279bc7 Mon Sep 17 00:00:00 2001
From: Martin Willi <[email protected]>
Date: Tue, 18 May 2010 12:20:32 +0200
Subject: [PATCH 1/3] Added simple conditional packet send delay
---
src/libcharon/network/sender.c | 41 ++++++++++++++++++++++++++++++++++++++-
1 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/src/libcharon/network/sender.c b/src/libcharon/network/sender.c
index c18f113..bb6d506 100644
--- a/src/libcharon/network/sender.c
+++ b/src/libcharon/network/sender.c
@@ -67,6 +67,21 @@ struct private_sender_t {
* Delay for sending outgoing packets, to simulate larger RTT
*/
int send_delay;
+
+ /**
+ * Specific message type to delay, 0 for any
+ */
+ int send_delay_type;
+
+ /**
+ * Delay request messages?
+ */
+ bool send_delay_request;
+
+ /**
+ * Delay response messages?
+ */
+ bool send_delay_response;
};
METHOD(sender_t, send_, void,
@@ -80,7 +95,23 @@ METHOD(sender_t, send_, void,
if (this->send_delay)
{
- usleep(this->send_delay * 1000);
+ message_t *message;
+
+ message = message_create_from_packet(packet->clone(packet));
+ if (message->parse_header(message) == SUCCESS)
+ {
+ if (this->send_delay_type == 0 ||
+ this->send_delay_type == message->get_exchange_type(message))
+ {
+ if ((message->get_request(message) && this->send_delay_request) ||
+ (!message->get_request(message) && this->send_delay_response))
+ {
+ DBG1(DBG_NET, "using send delay: %dms", this->send_delay);
+ usleep(this->send_delay * 1000);
+ }
+ }
+ }
+ message->destroy(message);
}
this->mutex->lock(this->mutex);
@@ -155,7 +186,13 @@ sender_t * sender_create()
.job = callback_job_create((callback_job_cb_t)send_packets,
this, NULL, NULL),
.send_delay = lib->settings->get_int(lib->settings,
- "charon.send_delay", 0),
+ "charon.send_delay", 0),
+ .send_delay_type = lib->settings->get_int(lib->settings,
+ "charon.send_delay_type", 0),
+ .send_delay_request = lib->settings->get_bool(lib->settings,
+ "charon.send_delay_request", TRUE),
+ .send_delay_response = lib->settings->get_int(lib->settings,
+ "charon.send_delay_response", TRUE),
);
charon->processor->queue_job(charon->processor, (job_t*)this->job);
--
1.7.0.4
>From d235274486e52ca2b8fc02fee541f316827906e3 Mon Sep 17 00:00:00 2001
From: Martin Willi <[email protected]>
Date: Tue, 18 May 2010 12:21:05 +0200
Subject: [PATCH 2/3] Added simple conditional packet receive delay
---
src/libcharon/network/receiver.c | 47 ++++++++++++++++++++++++++++++-------
1 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/src/libcharon/network/receiver.c b/src/libcharon/network/receiver.c
index df89702..bf6ccf7 100644
--- a/src/libcharon/network/receiver.c
+++ b/src/libcharon/network/receiver.c
@@ -103,7 +103,22 @@ struct private_receiver_t {
/**
* Delay for receiving incoming packets, to simulate larger RTT
*/
- u_int receive_delay;
+ int receive_delay;
+
+ /**
+ * Specific message type to delay, 0 for any
+ */
+ int receive_delay_type;
+
+ /**
+ * Delay request messages?
+ */
+ bool receive_delay_request;
+
+ /**
+ * Delay response messages?
+ */
+ bool receive_delay_response;
};
/**
@@ -259,7 +274,6 @@ static job_requeue_t receive_packets(private_receiver_t *this)
{
packet_t *packet;
message_t *message;
- job_t *job;
/* read in a packet */
if (charon->socket->receive(charon->socket, &packet) != SUCCESS)
@@ -329,16 +343,25 @@ static job_requeue_t receive_packets(private_receiver_t *this)
return JOB_REQUEUE_DIRECT;
}
}
- job = (job_t*)process_message_job_create(message);
if (this->receive_delay)
{
- charon->scheduler->schedule_job_ms(charon->scheduler,
- job, this->receive_delay);
- }
- else
- {
- charon->processor->queue_job(charon->processor, job);
+ if (this->receive_delay_type == 0 ||
+ this->receive_delay_type == message->get_exchange_type(message))
+ {
+ if ((message->get_request(message) && this->receive_delay_request) ||
+ (!message->get_request(message) && this->receive_delay_response))
+ {
+ DBG1(DBG_NET, "using receive delay: %dms",
+ this->receive_delay);
+ charon->scheduler->schedule_job_ms(charon->scheduler,
+ (job_t*)process_message_job_create(message),
+ this->receive_delay);
+ return JOB_REQUEUE_DIRECT;
+ }
+ }
}
+ charon->processor->queue_job(charon->processor,
+ (job_t*)process_message_job_create(message));
return JOB_REQUEUE_DIRECT;
}
@@ -374,6 +397,12 @@ receiver_t *receiver_create()
}
this->receive_delay = lib->settings->get_int(lib->settings,
"charon.receive_delay", 0);
+ this->receive_delay_type = lib->settings->get_int(lib->settings,
+ "charon.receive_delay_type", 0),
+ this->receive_delay_request = lib->settings->get_bool(lib->settings,
+ "charon.receive_delay_request", TRUE),
+ this->receive_delay_response = lib->settings->get_int(lib->settings,
+ "charon.receive_delay_response", TRUE),
this->hasher = lib->crypto->create_hasher(lib->crypto, HASH_PREFERRED);
if (this->hasher == NULL)
--
1.7.0.4
>From ea409980b9a3178b4860f2957685f7ec91956b1d Mon Sep 17 00:00:00 2001
From: Martin Willi <[email protected]>
Date: Tue, 18 May 2010 12:21:38 +0200
Subject: [PATCH 3/3] Handle collisions between rekey and the following delete properly
---
src/libcharon/sa/tasks/child_rekey.c | 90 +++++++++++++++++++++++----------
1 files changed, 63 insertions(+), 27 deletions(-)
diff --git a/src/libcharon/sa/tasks/child_rekey.c b/src/libcharon/sa/tasks/child_rekey.c
index b5e4e84..5331419 100644
--- a/src/libcharon/sa/tasks/child_rekey.c
+++ b/src/libcharon/sa/tasks/child_rekey.c
@@ -215,6 +215,59 @@ static status_t build_r(private_child_rekey_t *this, message_t *message)
}
/**
+ * Handle a rekey collision
+ */
+static child_sa_t *handle_collision(private_child_rekey_t *this)
+{
+ child_sa_t *to_delete;
+
+ if (this->collision->get_type(this->collision) == CHILD_REKEY)
+ {
+ chunk_t this_nonce, other_nonce;
+ private_child_rekey_t *other = (private_child_rekey_t*)this->collision;
+
+ this_nonce = this->child_create->get_lower_nonce(this->child_create);
+ other_nonce = other->child_create->get_lower_nonce(other->child_create);
+
+ /* if we have the lower nonce, delete rekeyed SA. If not, delete
+ * the redundant. */
+ if (memcmp(this_nonce.ptr, other_nonce.ptr,
+ min(this_nonce.len, other_nonce.len)) < 0)
+ {
+ DBG1(DBG_IKE, "CHILD_SA rekey collision won, "
+ "deleting rekeyed child");
+ to_delete = this->child_sa;
+ }
+ else
+ {
+ DBG1(DBG_IKE, "CHILD_SA rekey collision lost, "
+ "deleting redundant child");
+ to_delete = this->child_create->get_child(this->child_create);
+ }
+ }
+ else
+ { /* CHILD_DELETE */
+ child_delete_t *del = (child_delete_t*)this->collision;
+
+ /* we didn't had a chance to compare the nonces, so we delete
+ * the CHILD_SA the other is not deleting. */
+ if (del->get_child(del) != this->child_sa)
+ {
+ DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, "
+ "deleting rekeyed child");
+ to_delete = this->child_sa;
+ }
+ else
+ {
+ DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, "
+ "deleting redundant child");
+ to_delete = this->child_create->get_child(this->child_create);
+ }
+ }
+ return to_delete;
+}
+
+/**
* Implementation of task_t.process for initiator
*/
static status_t process_i(private_child_rekey_t *this, message_t *message)
@@ -263,35 +316,14 @@ static status_t process_i(private_child_rekey_t *this, message_t *message)
return SUCCESS;
}
- to_delete = this->child_sa;
-
/* check for rekey collisions */
- if (this->collision &&
- this->collision->get_type(this->collision) == CHILD_REKEY)
+ if (this->collision)
{
- chunk_t this_nonce, other_nonce;
- private_child_rekey_t *other = (private_child_rekey_t*)this->collision;
-
- this_nonce = this->child_create->get_lower_nonce(this->child_create);
- other_nonce = other->child_create->get_lower_nonce(other->child_create);
-
- /* if we have the lower nonce, delete rekeyed SA. If not, delete
- * the redundant. */
- if (memcmp(this_nonce.ptr, other_nonce.ptr,
- min(this_nonce.len, other_nonce.len)) < 0)
- {
- DBG1(DBG_IKE, "CHILD_SA rekey collision won, deleting rekeyed child");
- }
- else
- {
- DBG1(DBG_IKE, "CHILD_SA rekey collision lost, deleting redundant child");
- to_delete = this->child_create->get_child(this->child_create);
- if (to_delete == NULL)
- {
- /* ooops, should not happen, fallback */
- to_delete = this->child_sa;
- }
- }
+ to_delete = handle_collision(this);
+ }
+ else
+ {
+ to_delete = this->child_sa;
}
if (to_delete != this->child_create->get_child(this->child_create))
@@ -300,6 +332,10 @@ static status_t process_i(private_child_rekey_t *this, message_t *message)
this->child_create->get_child(this->child_create));
}
+ if (to_delete == NULL)
+ {
+ return SUCCESS;
+ }
spi = to_delete->get_spi(to_delete, TRUE);
protocol = to_delete->get_protocol(to_delete);
--
1.7.0.4
_______________________________________________
Dev mailing list
[email protected]
https://lists.strongswan.org/mailman/listinfo/dev
