Fixed a race condition when querying stats of a child_sa in different order.

src/charon/plugins/stroke/stroke_list.c

@@ -146,8 +146,7 @@ static void log_ike_sa(FILE *out, ike_sa_t *ike_sa, bool all)
  */
 static void log_child_sa(FILE *out, child_sa_t *child_sa, bool all)
 {
-	u_int32_t rekey, now = time(NULL);
+	time_t use_in, use_out, rekey, now = time(NULL);
-	u_int32_t use_in, use_out;
 	u_int64_t bytes_in, bytes_out;
 	proposal_t *proposal;
 	child_cfg_t *config = child_sa->get_config(child_sa);
@@ -207,26 +206,18 @@ static void log_child_sa(FILE *out, child_sa_t *child_sa, bool all)
 				}
 			}
 
-			bytes_in = child_sa->get_usebytes(child_sa, TRUE);
+			child_sa->get_usestats(child_sa, TRUE, &use_in, &bytes_in);
 			fprintf(out, ", %lu bytes_i", bytes_in);
-			if (bytes_in)
+			if (use_in)
 			{
-				use_in = child_sa->get_usetime(child_sa, TRUE);
+				fprintf(out, " (%ds ago)", now - use_in);
-				if (use_in)
-				{
-					fprintf(out, " (%ds ago)", now - use_in);
-				}
 			}
 
-			bytes_out = child_sa->get_usebytes(child_sa, FALSE);
+			child_sa->get_usestats(child_sa, FALSE, &use_out, &bytes_out);
 			fprintf(out, ", %lu bytes_o", bytes_out);
-			if (bytes_out)
+			if (use_out)
 			{
-				use_out = child_sa->get_usetime(child_sa, FALSE);
+				fprintf(out, " (%ds ago)", now - use_out);
-				if (use_out)
-				{
-					fprintf(out, " (%ds ago)", now - use_out);
-				}
 			}
 			fprintf(out, ", rekeying ");
 			

src/charon/sa/child_sa.c

@@ -380,7 +380,7 @@ static enumerator_t* create_policy_enumerator(private_child_sa_t *this)
  * are available, and NOT_SUPPORTED if the kernel interface does not support
  * quering the usebytes.
  */
-static bool update_usebytes(private_child_sa_t *this, bool inbound)
+static status_t update_usebytes(private_child_sa_t *this, bool inbound)
 {
 	status_t status = FAILED;
 	u_int64_t bytes;
@@ -427,19 +427,14 @@ static bool update_usebytes(private_child_sa_t *this, bool inbound)
 }
 
 /**
- * Implementation of child_sa_t.get_usetime
+ * updates the cached usetime
  */
-static u_int32_t get_usetime(private_child_sa_t *this, bool inbound)
+static void update_usetime(private_child_sa_t *this, bool inbound)
 {
 	enumerator_t *enumerator;
 	traffic_selector_t *my_ts, *other_ts;
 	u_int32_t last_use = 0;
 	
-	if (update_usebytes(this, inbound) == FAILED)
-	{	/* no SPI or no traffic since last update */
-		return inbound ? this->my_usetime : this->other_usetime;
-	}
-	
 	enumerator = create_policy_enumerator(this);
 	while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 	{
@@ -479,16 +474,26 @@ static u_int32_t get_usetime(private_child_sa_t *this, bool inbound)
 	{
 		this->other_usetime = last_use;
 	}
-	return last_use;
 }
 
 /**
- * Implementation of child_sa_t.get_usebytes
+ * Implementation of child_sa_t.get_usestats
  */
-static u_int64_t get_usebytes(private_child_sa_t *this, bool inbound)
+static void get_usestats(private_child_sa_t *this, bool inbound,
+						 time_t *time, u_int64_t *bytes)
 {
-	update_usebytes(this, inbound);
+	if (update_usebytes(this, inbound) != FAILED)
-	return inbound ? this->my_usebytes : this->other_usebytes;
+	{	/* there was traffic since last update or the KI does not support it */
+		update_usetime(this, inbound);
+	}
+	if (time)
+	{
+		*time = inbound ? this->my_usetime : this->other_usetime;
+	}
+	if (bytes)
+	{
+		*bytes = inbound ? this->my_usebytes : this->other_usebytes;
+	}
 }
 
 /**
@@ -869,8 +874,7 @@ child_sa_t * child_sa_create(host_t *me, host_t* other,
 	this->public.get_proposal = (proposal_t*(*)(child_sa_t*))get_proposal;
 	this->public.set_proposal = (void(*)(child_sa_t*, proposal_t *proposal))set_proposal;
 	this->public.get_lifetime = (u_int32_t(*)(child_sa_t*, bool))get_lifetime;
-	this->public.get_usetime = (u_int32_t(*)(child_sa_t*, bool))get_usetime;
+	this->public.get_usestats = (void(*)(child_sa_t*,bool,time_t*,u_int64_t*))get_usestats;
-	this->public.get_usebytes = (u_int64_t(*)(child_sa_t*, bool))get_usebytes;
 	this->public.has_encap = (bool(*)(child_sa_t*))has_encap;
 	this->public.get_ipcomp = (ipcomp_transform_t(*)(child_sa_t*))get_ipcomp;
 	this->public.set_ipcomp = (void(*)(child_sa_t*,ipcomp_transform_t))set_ipcomp;

src/charon/sa/child_sa.h

@@ -85,11 +85,11 @@ extern enum_name_t *child_sa_state_names;
 
 /**
  * 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
  * SAs and the policies have the same reqid.
- * 
+ *
  * The procedure for child sa setup is as follows:
  * - A gets SPIs for a all protocols in its proposals via child_sa_t.alloc
  * - A send the proposals with the allocated SPIs to B
@@ -98,7 +98,7 @@ extern enum_name_t *child_sa_state_names;
  * - B calls child_sa_t.install for both, the allocated and received SPI
  * - B sends the proposal with the allocated SPI to A
  * - A calls child_sa_t.install for both, the allocated and recevied SPI
- * 
+ *
  * Once SAs are set up, policies can be added using add_policies.
  */
 struct child_sa_t {
@@ -112,7 +112,7 @@ struct child_sa_t {
 	
 	/**
 	 * Get the reqid of the CHILD SA.
-	 * 
+	 *
 	 * Every CHILD_SA has a reqid. The kernel uses this ID to
 	 * identify it.
 	 *
@@ -131,19 +131,19 @@ struct child_sa_t {
 	 * Get the state of the CHILD_SA.
 	 *
 	 * @return 			CHILD_SA state
-	 */	
+	 */
 	child_sa_state_t (*get_state) (child_sa_t *this);
 	
 	/**
 	 * Set the state of the CHILD_SA.
 	 *
 	 * @param state		state to set on CHILD_SA
-	 */	
+	 */
 	void (*set_state) (child_sa_t *this, child_sa_state_t state);
 	
 	/**
 	 * Get the SPI of this CHILD_SA.
-	 * 
+	 *
 	 * Set the boolean parameter inbound to TRUE to
 	 * get the SPI for which we receive packets, use
 	 * FALSE to get those we use for sending packets.
@@ -155,7 +155,7 @@ struct child_sa_t {
 	
 	/**
 	 * Get the CPI of this CHILD_SA.
-	 * 
+	 *
 	 * Set the boolean parameter inbound to TRUE to
 	 * get the CPI for which we receive packets, use
 	 * FALSE to get those we use for sending packets.
@@ -202,7 +202,7 @@ struct child_sa_t {
 	
 	/**
 	 * Set the IPComp algorithm to use.
-	 * 
+	 *
 	 * @param ipcomp	the IPComp transform to use
 	 */
 	void (*set_ipcomp)(child_sa_t *this, ipcomp_transform_t ipcomp);
@@ -219,7 +219,7 @@ struct child_sa_t {
 	 *
 	 * @param proposal	selected proposal
 	 */
-	void (*set_proposal)(child_sa_t *this, proposal_t *proposal);	
+	void (*set_proposal)(child_sa_t *this, proposal_t *proposal);
 	
 	/**
 	 * Check if this CHILD_SA uses UDP encapsulation.
@@ -237,27 +237,21 @@ struct child_sa_t {
 	u_int32_t (*get_lifetime)(child_sa_t *this, bool hard);
 	
 	/**
-	 * Get last use time of the CHILD_SA.
+	 * Get last use time and the number of bytes processed.
-	 *
-	 * @param inbound	TRUE for inbound traffic, FALSE for outbound
-	 * @return			time of last use in seconds
-	 */
-	u_int32_t (*get_usetime)(child_sa_t *this, bool inbound);
-	
-	/**
-	 * Get number of bytes processed by CHILD_SA.
 	 *
 	 * @param inbound		TRUE for inbound traffic, FALSE for outbound
-	 * @return				number of processed bytes
+	 * @param[out] time		time of last use in seconds (NULL to ignore)
+	 * @param[out] bytes	number of processed bytes (NULL to ignore)
 	 */
-	u_int64_t (*get_usebytes)(child_sa_t *this, bool inbound);
+	void (*get_usestats)(child_sa_t *this, bool inbound, time_t *time,
-
+						 u_int64_t *bytes);
+	
 	/**
 	 * Get the traffic selectors list added for one side.
 	 *
 	 * @param local		TRUE for own traffic selectors, FALSE for remote
 	 * @return			list of traffic selectors
-	 */	
+	 */
 	linked_list_t* (*get_traffic_selectors) (child_sa_t *this, bool local);
 	
 	/**
@@ -304,7 +298,7 @@ struct child_sa_t {
 	 * @param my_ts		traffic selectors for local site
 	 * @param other_ts	traffic selectors for remote site
 	 * @return			SUCCESS or FAILED
-	 */	
+	 */
 	status_t (*add_policies)(child_sa_t *this, linked_list_t *my_ts_list,
 							 linked_list_t *other_ts_list);
 	/**

src/charon/sa/ike_sa.c

@@ -260,7 +260,7 @@ static time_t get_use_time(private_ike_sa_t* this, bool inbound)
 {
 	enumerator_t *enumerator;
 	child_sa_t *child_sa;
-	time_t use_time;
+	time_t use_time, current;
 	
 	if (inbound)
 	{
@@ -273,7 +273,8 @@ static time_t get_use_time(private_ike_sa_t* this, bool inbound)
 	enumerator = this->child_sas->create_enumerator(this->child_sas);
 	while (enumerator->enumerate(enumerator, &child_sa))
 	{
-		use_time = max(use_time, child_sa->get_usetime(child_sa, inbound));
+		child_sa->get_usestats(child_sa, inbound, &current, NULL);
+		use_time = max(use_time, current);
 	}
 	enumerator->destroy(enumerator);
 	

src/charon/sa/tasks/child_delete.c

@@ -240,17 +240,19 @@ static void log_children(private_child_delete_t *this)
 {
 	iterator_t *iterator;
 	child_sa_t *child_sa;
+	u_int64_t bytes_in, bytes_out;
 	
 	iterator = this->child_sas->create_iterator(this->child_sas, TRUE);
 	while (iterator->iterate(iterator, (void**)&child_sa))
 	{
+		child_sa->get_usestats(child_sa, TRUE, NULL, &bytes_in);
+		child_sa->get_usestats(child_sa, FALSE, NULL, &bytes_out);
+		
 		DBG0(DBG_IKE, "closing CHILD_SA %s{%d} "
 			 "with SPIs %.8x_i (%lu bytes) %.8x_o (%lu bytes) and TS %#R=== %#R",
 			 child_sa->get_name(child_sa), child_sa->get_reqid(child_sa),
-			 ntohl(child_sa->get_spi(child_sa, TRUE)),
+			 ntohl(child_sa->get_spi(child_sa, TRUE)), bytes_in,
-			 child_sa->get_usebytes(child_sa, TRUE),
+			 ntohl(child_sa->get_spi(child_sa, FALSE)), bytes_out,
-			 ntohl(child_sa->get_spi(child_sa, FALSE)),
-			 child_sa->get_usebytes(child_sa, FALSE),
 			 child_sa->get_traffic_selectors(child_sa, TRUE),
 			 child_sa->get_traffic_selectors(child_sa, FALSE));
 	}