further work done for simultaneous rekeying/delete

still some cases which cause trouble

src/charon/sa/child_sa.c

@@ -110,9 +110,14 @@ struct private_child_sa_t {
 	u_int32_t hard_lifetime;
 	
 	/**
-	 * has this CHILD_SA been rekeyed?
+	 * transaction which is rekeying this CHILD_SA
 	 */
-	bool rekeyed;
+	void *rekeying_transaction;
+	
+	/**
+	 * has this child SA been rekeyed/is rekeying?
+	 */
+	bool is_rekeying;
 
 	/**
 	 * Specifies if NAT traversal is used
@@ -322,7 +327,7 @@ static status_t install(private_child_sa_t *this, proposal_t *proposal, prf_plus
 											  prf_plus, natt, mine);
 	
 	this->install_time = time(NULL);
-
+	
 	return status;
 }
 
@@ -477,11 +482,28 @@ static status_t add_policies(private_child_sa_t *this, linked_list_t *my_ts_list
 }
 
 /**
- * Implementation of child_sa_t.set_rekeyed.
+ * Implementation of child_sa_t.set_rekeying_transaction.
  */
-static void set_rekeyed(private_child_sa_t *this)
+static void set_rekeying_transaction(private_child_sa_t *this, void *transaction)
 {
-	this->rekeyed = TRUE;
+	this->rekeying_transaction = transaction;
+	this->is_rekeying = TRUE;
+}
+
+/**
+ * Implementation of child_sa_t.get_rekeying_transaction.
+ */
+static void* get_rekeying_transaction(private_child_sa_t *this)
+{
+	return this->rekeying_transaction;
+}
+
+/**
+ * Implementation of child_sa_t.is_rekeying.
+ */
+static bool is_rekeying(private_child_sa_t *this)
+{
+	return this->is_rekeying;
 }
 
 /**
@@ -684,16 +706,6 @@ static status_t update_sa_hosts(private_child_sa_t *this, host_t *new_me, host_t
 	u_int32_t spi;
 	
 	if (mine)
-	{
-		src = this->me.addr;
-		dst = this->other.addr;
-		new_src = new_me;
-		new_dst = new_other;
-		src_changes = my_changes;
-		dst_changes = other_changes;
-		spi = this->me.spi;
-	}
-	else
 	{
 		src = this->other.addr;
 		dst = this->me.addr;
@@ -703,6 +715,16 @@ static status_t update_sa_hosts(private_child_sa_t *this, host_t *new_me, host_t
 		dst_changes = my_changes;
 		spi = this->other.spi;
 	}
+	else
+	{
+		src = this->me.addr;
+		dst = this->other.addr;
+		new_src = new_me;
+		new_dst = new_other;
+		src_changes = my_changes;
+		dst_changes = other_changes;
+		spi = this->me.spi;
+	}
 	
 	this->logger->log(this->logger, CONTROL|LEVEL1,
 					  "updating %s SA 0x%x, from %s:%d..%s:%d to %s:%d..%s:%d",
@@ -783,7 +805,7 @@ static status_t update_policy_hosts(private_child_sa_t *this, host_t *new_me, ho
 static status_t update_hosts(private_child_sa_t *this, host_t *new_me, host_t *new_other, 
 							 int my_changes, int other_changes) 
 {
-	if (!my_changes || !other_changes)
+	if (!my_changes && !other_changes)
 	{
 		return SUCCESS;
 	}
@@ -857,7 +879,7 @@ static void destroy(private_child_sa_t *this)
 	/* delete all policies in the kernel */
 	while (this->policies->remove_last(this->policies, (void**)&policy) == SUCCESS)
 	{
-		if (!this->rekeyed)
+		if (!this->is_rekeying)
 		{	
 			/* let rekeyed policies, as they are used by another child_sa */
 			charon->kernel_interface->del_policy(charon->kernel_interface,
@@ -883,7 +905,9 @@ static void destroy(private_child_sa_t *this)
 		free(policy);
 	}
 	this->policies->destroy(this->policies);
-
+	
+	this->me.addr->destroy(this->me.addr);
+	this->other.addr->destroy(this->other.addr);
 	free(this);
 }
 
@@ -907,14 +931,16 @@ child_sa_t * child_sa_create(u_int32_t rekey, host_t *me, host_t* other,
 	this->public.update_hosts = (status_t (*)(child_sa_t*,host_t*,host_t*,int,int))update_hosts;
 	this->public.add_policies = (status_t (*)(child_sa_t*, linked_list_t*,linked_list_t*))add_policies;
 	this->public.get_use_time = (status_t (*)(child_sa_t*,bool,time_t*))get_use_time;
-	this->public.set_rekeyed = (void (*)(child_sa_t*))set_rekeyed;
+	this->public.set_rekeying_transaction = (void (*)(child_sa_t*,void*))set_rekeying_transaction;
+	this->public.get_rekeying_transaction = (void* (*)(child_sa_t*))get_rekeying_transaction;
+	this->public.is_rekeying = (bool (*)(child_sa_t*))is_rekeying;
 	this->public.log_status = (void (*)(child_sa_t*, logger_t*, char*))log_status;
 	this->public.destroy = (void(*)(child_sa_t*))destroy;
 
 	/* private data */
 	this->logger = logger_manager->get_logger(logger_manager, CHILD_SA);
-	this->me.addr = me;
-	this->other.addr = other;
+	this->me.addr = me->clone(me);
+	this->other.addr = other->clone(other);
 	this->me.spi = 0;
 	this->other.spi = 0;
 	this->alloc_ah_spi = 0;
@@ -926,7 +952,8 @@ child_sa_t * child_sa_create(u_int32_t rekey, host_t *me, host_t* other,
 	this->reqid = rekey ? rekey : ++reqid;
 	this->policies = linked_list_create();
 	this->protocol = PROTO_NONE;
-	this->rekeyed = FALSE;
+	this->rekeying_transaction = NULL;
+	this->is_rekeying = FALSE;
 	
 	return &this->public;
 }

src/charon/sa/child_sa.h

@@ -39,7 +39,7 @@
 typedef struct child_sa_t child_sa_t;
 
 /**
- * @brief Represents multiple IPsec SAs between two hosts.
+ * @brief Represents an IPsec SAs between two hosts.
  * 
  * A child_sa_t contains two SAs. SAs for both
  * directions are managed in one child_sa_t object. Both
@@ -168,15 +168,35 @@ struct child_sa_t {
 	status_t (*get_use_time) (child_sa_t *this, bool inbound, time_t *use_time);
 	
 	/**
-	 * @brief Mark this child_sa as rekeyed.
+	 * @brief Set the transaction which rekeys this CHILD_SA.
 	 *
-	 * Since an SA which rekeys a old SA shares the same policy,
-	 * we must mark a child_sa as rekeyed. A so marked SA does
-	 * not remove its policy, as the new SA uses it.
+	 * Since either end may initiate CHILD_SA rekeying, we must detect
+	 * such situations to handle them cleanly. A rekeying transaction
+	 * registers itself to the CHILD_SA, and checks later if another
+	 * transaction is in progress of a rekey.
 	 *
 	 * @param this 		calling object
 	 */	
-	void (*set_rekeyed) (child_sa_t *this);
+	void (*set_rekeying_transaction) (child_sa_t *this, void *transaction);
+	
+	/**
+	 * @brief Get the transaction which rekeys this CHILD_SA.
+	 *
+	 * See set_rekeying_transactoin
+	 *
+	 * @param this 		calling object
+	 */	
+	void* (*get_rekeying_transaction) (child_sa_t *this);
+	
+	/**
+	 * @brief Is the CHILD SA rekeying/in progress of rekeying?
+	 *
+	 * This is a readonly parameter. It is set whenever the 
+	 * set_rekeying_transaction() method is called.
+	 *
+	 * @param this 		calling object
+	 */	
+	bool (*is_rekeying) (child_sa_t *this);
 	
 	/**
 	 * @brief Log the status of a child_sa to a logger.

src/charon/sa/transactions/create_child_sa.c

@@ -28,7 +28,6 @@
 #include <encoding/payloads/sa_payload.h>
 #include <encoding/payloads/nonce_payload.h>
 #include <encoding/payloads/ts_payload.h>
-#include <sa/child_sa.h>
 #include <sa/transactions/delete_child_sa.h>
 #include <utils/randomizer.h>
 
@@ -115,6 +114,11 @@ struct private_create_child_sa_t {
 	 */
 	child_sa_t *rekeyed_sa;
 	
+	/**
+	 * Have we lost the simultaneous rekeying nonce compare?
+	 */
+	bool lost;
+	
 	/**
 	 * source of randomness
 	 */
@@ -143,13 +147,22 @@ static u_int32_t requested(private_create_child_sa_t *this)
 }
 
 /**
- * Implementation of transaction_t.rekeys_child.
+ * Implementation of create_child_sa_t.rekeys_child.
  */
 static void rekeys_child(private_create_child_sa_t *this, child_sa_t *child_sa)
 {
 	this->rekeyed_sa = child_sa;
 }
 
+/**
+ * Implementation of create_child_sa_t.cancel.
+ */
+static void cancel(private_create_child_sa_t *this)
+{
+	this->rekeyed_sa = NULL;
+	this->lost = TRUE;
+}
+
 /**
  * Implementation of transaction_t.get_request.
  */
@@ -158,6 +171,15 @@ static status_t get_request(private_create_child_sa_t *this, message_t **result)
 	message_t *request;
 	host_t *me, *other;
 	
+	/* check if we are not already rekeying */
+	if (this->rekeyed_sa &&
+		this->rekeyed_sa->is_rekeying(this->rekeyed_sa))
+	{
+		this->logger->log(this->logger, ERROR,
+						  "rekeying a CHILD_SA which is already rekeying, aborted");
+		return FAILED;
+	}
+	
 	/* check if we already have built a message (retransmission) */
 	if (this->message)
 	{
@@ -249,8 +271,8 @@ static status_t get_request(private_create_child_sa_t *this, message_t **result)
 		notify->set_spi(notify, this->rekeyed_sa->get_spi(this->rekeyed_sa, TRUE));
 		request->add_payload(request, (payload_t*)notify);
 		
-		/* and mark sa with rekeying-in-progress */
-		this->rekeyed_sa->set_rekeyed(this->rekeyed_sa);
+		/* register us as rekeying to detect multiple rekeying */
+		this->rekeyed_sa->set_rekeying_transaction(this->rekeyed_sa, &this->public);
 	}
 	
 	return SUCCESS;
@@ -605,12 +627,21 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
 		ts_response = ts_payload_create_from_traffic_selectors(FALSE, this->tsr);
 		response->add_payload(response, (payload_t*)ts_response);
 	}
-	/* CHILD_SA successfully created. Now we must check if it rekeys an old one, 
-	 * and if so, mark the old as rekeyed. It will get deleted from the other
-	 * peer. */
+	/* CHILD_SA successfully created. We set us as the rekeying transaction of
+	 * this SA. If we already initiated rekeying of the same SA, we will detect
+	 * this later in the conclude() call. */
 	if (this->rekeyed_sa)
 	{
-		this->rekeyed_sa->set_rekeyed(this->rekeyed_sa);
+		if (this->rekeyed_sa->is_rekeying(this->rekeyed_sa))
+		{
+			/* rekeying already in progress, register us, too */
+			this->rekeyed_sa->set_rekeying_transaction(this->rekeyed_sa, &this->public);
+		}
+		else
+		{
+			/* no rekeying in progress. mark SA as rekeyed, but not conflicted */
+			this->rekeyed_sa->set_rekeying_transaction(this->rekeyed_sa, NULL);
+		}
 	}
 	return SUCCESS;
 }
@@ -628,6 +659,8 @@ static status_t conclude(private_create_child_sa_t *this, message_t *response,
 	ts_payload_t *tsi_payload = NULL;
 	ts_payload_t *tsr_payload = NULL;
 	status_t status;
+	child_sa_t *new_child = NULL;
+	delete_child_sa_t *delete_child_sa;
 	
 	/* check message type */
 	if (response->get_exchange_type(response) != CREATE_CHILD_SA)
@@ -726,6 +759,7 @@ static status_t conclude(private_create_child_sa_t *this, message_t *response,
 							  "CHILD_SA creation failed");
 			return FAILED;
 		}
+		new_child = this->child_sa;
 		if (install_child_sa(this, TRUE) != SUCCESS)
 		{
 			this->logger->log(this->logger, ERROR,
@@ -733,14 +767,73 @@ static status_t conclude(private_create_child_sa_t *this, message_t *response,
 			return FAILED;
 		}
 	}
-	/* CHILD_SA successfully created. If we have rekeyed an old one, delete it */
+	/* CHILD_SA successfully created. If the other peer initiated rekeying
+	 * in the meantime, we detect this by comparing the rekeying_transaction
+	 * of the SA. If it changed, we are not alone. Then we must compare the nonces.
+	 * If no simultaneous rekeying is going on, we just initiate the delete of
+	 * the superseded SA. */
 	if (this->rekeyed_sa)
 	{
-		delete_child_sa_t *delete_child_sa;
+		private_create_child_sa_t *other;
 		
-		delete_child_sa = delete_child_sa_create(this->ike_sa, 
-												 this->message_id + 1);
-		delete_child_sa->set_child_sa(delete_child_sa, this->rekeyed_sa);
+		other = (private_create_child_sa_t*)
+					this->rekeyed_sa->get_rekeying_transaction(this->rekeyed_sa);
+		
+		/* we are not rekeying anymore, unregister us */
+		this->rekeyed_sa->set_rekeying_transaction(this->rekeyed_sa, NULL);
+		
+		if (other != this)
+		{	/* simlutaneous rekeying is going on, not so good */
+			chunk_t this_lowest, other_lowest;
+			
+			/* check if this has a lower nonce the other */
+			if (memcmp(this->nonce_i.ptr, this->nonce_r.ptr, 
+				min(this->nonce_i.len, this->nonce_r.len)) < 0)
+			{
+				this_lowest = this->nonce_i;
+			}
+			else
+			{
+				this_lowest = this->nonce_r;
+			}
+			if (memcmp(other->nonce_i.ptr, other->nonce_r.ptr, 
+				min(other->nonce_i.len, other->nonce_r.len)) < 0)
+			{
+				other_lowest = other->nonce_i;
+			}
+			else
+			{
+				other_lowest = other->nonce_r;
+			}
+			if (memcmp(this_lowest.ptr, other_lowest.ptr, 
+				min(this_lowest.len, other_lowest.len)) < 0)
+			{
+				this->logger->log(this->logger, ERROR,
+								  "detected simultaneous CHILD_SA rekeying, but ours is preferred");
+			}
+			else
+			{
+				
+				this->logger->log(this->logger, ERROR,
+								  "detected simultaneous CHILD_SA rekeying, deleting ours");
+				this->lost = TRUE;
+			}
+		}	
+		/* delete the old SA if we have won the rekeying nonce compare*/
+		if (!this->lost)
+		{
+			other->rekeyed_sa->set_rekeying_transaction(other->rekeyed_sa, NULL);
+			delete_child_sa = delete_child_sa_create(this->ike_sa, this->message_id + 1);
+			delete_child_sa->set_child_sa(delete_child_sa, this->rekeyed_sa);
+			*next = (transaction_t*)delete_child_sa;
+		}
+	}
+	if (this->lost)
+	{
+		/* we have lost simlutaneous rekeying, delete the CHILD_SA we just have created */
+		delete_child_sa = delete_child_sa_create(this->ike_sa, this->message_id + 1);
+		new_child->set_rekeying_transaction(new_child, NULL);
+		delete_child_sa->set_child_sa(delete_child_sa, new_child);
 		*next = (transaction_t*)delete_child_sa;
 	}
 	return SUCCESS;
@@ -788,6 +881,7 @@ create_child_sa_t *create_child_sa_create(ike_sa_t *ike_sa, u_int32_t message_id
 	
 	/* public functions */
 	this->public.rekeys_child = (void(*)(create_child_sa_t*,child_sa_t*))rekeys_child;
+	this->public.cancel = (void(*)(create_child_sa_t*))cancel;
 	
 	/* private data */
 	this->ike_sa = ike_sa;
@@ -799,6 +893,7 @@ create_child_sa_t *create_child_sa_create(ike_sa_t *ike_sa, u_int32_t message_id
 	this->nonce_r = CHUNK_INITIALIZER;
 	this->child_sa = NULL;
 	this->rekeyed_sa = NULL;
+	this->lost = FALSE;
 	this->proposal = NULL;
 	this->tsi = NULL;
 	this->tsr = NULL;

src/charon/sa/transactions/create_child_sa.h

@@ -20,9 +20,8 @@
  * for more details.
  */
 
-
-#ifndef CREATE_CHILD_SA_
-#define CREATE_CHILD_SA_
+#ifndef CREATE_CHILD_SA_H_
+#define CREATE_CHILD_SA_H_
 
 #include <sa/ike_sa.h>
 #include <sa/child_sa.h>
@@ -37,6 +36,23 @@ typedef struct create_child_sa_t create_child_sa_t;
  * Rekeying of an CHILD_SA works the same way as creating a new one,
  * but includes an additional REKEY_SA notify and deletes the old
  * one (in a separate transaction).
+ * 
+ *                     ¦__________  _________¦
+ *                     ¦  Cyq     \/    Czq  ¦
+ *                     ¦__________/\_________¦
+ *              detect ¦__________  _________¦ detect
+ *                     ¦  Czp     \/    Czp  ¦
+ * compare nonces, won ¦__________/\_________¦ compare nonces, lost
+ *                     ¦                     ¦
+ *        delete old   ¦__________           ¦
+ *                     ¦  Dxq     \__________¦
+ *                     ¦           __________¦
+ *                     ¦__________/    Dxp   ¦
+ *                     ¦           __________¦ delete created
+ *                     ¦__________/    Dzq   ¦
+ *                     ¦__________           ¦
+ *                     ¦  Dzp     \__________¦
+ * 
  *
  * @b Constructors:
  *  - create_child_sa_create()
@@ -61,6 +77,18 @@ struct create_child_sa_t {
 	 * @param child_sa	CHILD_SA to rekey
 	 */
 	void (*rekeys_child) (create_child_sa_t* this, child_sa_t *child_sa);
+	
+	/**
+	 * @brief Cancel a rekeying request.
+	 *
+	 * Cancelling a rekeying request will set a flag in the transaction. When
+	 * the response for the transaction is received, the created CHILD_SA
+	 * gets deleted afterwards.
+	 *
+	 * @param this		calling object
+	 * @param child_sa	CHILD_SA to rekey
+	 */
+	void (*cancel) (create_child_sa_t* this);
 };
 
 /**
@@ -74,4 +102,4 @@ struct create_child_sa_t {
  */
 create_child_sa_t *create_child_sa_create(ike_sa_t *ike_sa, u_int32_t message_id);
 
-#endif /* CREATE_CHILD_SA_ */
+#endif /* CREATE_CHILD_SA_H_ */

src/charon/sa/transactions/delete_child_sa.c

@@ -24,6 +24,7 @@
 
 #include <daemon.h>
 #include <encoding/payloads/delete_payload.h>
+#include <sa/transactions/create_child_sa.h>
 
 
 typedef struct private_delete_child_sa_t private_delete_child_sa_t;
@@ -151,7 +152,7 @@ static status_t process_delete(private_delete_child_sa_t *this, delete_payload_t
 	protocol_id_t protocol;
 	u_int32_t spi;
 	iterator_t *iterator;
-	delete_payload_t *delete_response;
+	delete_payload_t *delete_response = NULL;
 	
 	/* get requested CHILD */
 	protocol = delete_request->get_protocol_id(delete_request);
@@ -178,9 +179,23 @@ static status_t process_delete(private_delete_child_sa_t *this, delete_payload_t
 		
 		if (child_sa != NULL)
 		{
+			create_child_sa_t *rekey;
+			
 			this->logger->log(this->logger, CONTROL,
-							  "received DELETE for %s CHILD_SA with SPI 0x%x, deleting", 
+							  "received DELETE for %s CHILD_SA with SPI 0x%x, deleting",
 							  mapping_find(protocol_id_m, protocol), ntohl(spi));
+			
+			rekey = child_sa->get_rekeying_transaction(child_sa);
+			if (rekey)
+			{
+				/* we have received a delete for an SA which we are still rekeying.
+				 * this means we have lost the nonce comparison, and the rekeying
+				 * will fail. We set a flag in the transaction for this special case.
+				 */
+				this->logger->log(this->logger, CONTROL,
+								  "DELETE received while rekeying, rekeying cancelled");
+				rekey->cancel(rekey);
+			}
 			/* delete it, with inbound spi */
 			spi = child_sa->get_spi(child_sa, TRUE);
 			this->ike_sa->destroy_child_sa(this->ike_sa, protocol, spi);

src/charon/sa/transactions/transaction.h

@@ -23,12 +23,14 @@
 #ifndef TRANSACTION_H_
 #define TRANSACTION_H_
 
+
+typedef struct transaction_t transaction_t;
+
 #include <types.h>
 #include <encoding/message.h>
 #include <sa/ike_sa.h>
 
 
-typedef struct transaction_t transaction_t;
 
 /**
  * @brief This interface represents a transaction an established IKE_SA can do.