Issue #113, part four

There were quite a few small errors synchronizing the session timeouts.
Also removed the sessions' creation time, since it's no longer being
used.

I think all that's left is to figure out where did Rob came up with
2048 as maximum packet size limit and try a better number which does
not probably induce fragmentation. It should be ready after that.

include/nat64/common/session.h

@@ -2,7 +2,7 @@
 #define _JOOL_COMMON_SESSION_H
 
 /** The states from the TCP state machine; RFC 6146 section 3.5.2. */
-enum tcp_state {
+typedef enum tcp_state {
 	/** No traffic has been seen; state is fictional. */
 	CLOSED = 0,
 	/** A SYN packet arrived from the IPv6 side; some IPv4 node is trying to start a connection. */
@@ -25,6 +25,6 @@ enum tcp_state {
 	V4_FIN_V6_FIN_RCV,
 	/** The session might die in a short while. */
 	TRANS,
-};
+} tcp_state;
 
 #endif /* _JOOL_COMMON_SESSION_H */

include/nat64/common/xlat.h

@@ -13,7 +13,7 @@
 #define JOOL_VERSION_MAJOR 3
 #define JOOL_VERSION_MINOR 4
 #define JOOL_VERSION_REV 2
-#define JOOL_VERSION_DEV 7
+#define JOOL_VERSION_DEV 8
 
 /** See http://stackoverflow.com/questions/195975 */
 #define STR_VALUE(arg) #arg

include/nat64/mod/stateful/session/db.h

@@ -38,7 +38,7 @@ int sessiondb_find(struct sessiondb *db, struct tuple *tuple,
 		fate_cb cb, void *cb_arg,
 		struct session_entry **result);
 int sessiondb_add(struct sessiondb *db, struct session_entry *session,
-		fate_cb cb, void *cb_args);
+		fate_cb cb, void *cb_args, bool est_timer);
 
 int sessiondb_foreach(struct sessiondb *db, l4_protocol proto,
 		int (*func)(struct session_entry *, void *), void *arg,

include/nat64/mod/stateful/session/entry.h

@@ -4,6 +4,7 @@
 #include <linux/kref.h>
 #include <linux/rbtree.h>
 #include "nat64/common/types.h"
+#include "nat64/common/session.h"
 #include "nat64/mod/stateful/bib/db.h"
 
 /**
@@ -49,13 +50,11 @@ struct session_entry {
 
 	/** Transport protocol of the connection this session describes. */
 	const l4_protocol l4_proto;
-	/** Current TCP state. Only relevant if @l4_proto == L4PROTO_TCP. */
-	u_int8_t state;
+	/** Current TCP SM state. Only relevant if @l4_proto == L4PROTO_TCP. */
+	tcp_state state;
 
 	/** Jiffy (from the epoch) this session was last updated/used. */
 	unsigned long update_time;
-	/** Jiffy at which this session was created. */
-	unsigned long creation_time;
 
 	/**
 	 * When the session is in the database, this chains it to its

include/nat64/mod/stateful/session/table.h

@@ -39,6 +39,7 @@ typedef unsigned long (*timeout_cb)(struct global_config *);
 struct expire_timer {
 	struct list_head sessions;
 	unsigned long timeout;
+	bool is_established;
 	fate_cb decide_fate_cb;
 };
 
@@ -91,7 +92,7 @@ int sessiontable_find(struct session_table *table, struct tuple *tuple,
 		fate_cb cb, void *cb_arg,
 		struct session_entry **result);
 int sessiontable_add(struct session_table *table, struct session_entry *session,
-		fate_cb cb, void *cb_args);
+		fate_cb cb, void *cb_args, bool est);
 
 int sessiontable_foreach(struct session_table *table,
 		int (*func)(struct session_entry *, void *), void *arg,

mod/stateful/filtering_and_updating.c

@@ -1,6 +1,5 @@
 #include "nat64/mod/stateful/filtering_and_updating.h"
 
-#include "nat64/common/session.h"
 #include "nat64/common/str_utils.h"
 #include "nat64/mod/common/config.h"
 #include "nat64/mod/common/icmp_wrapper.h"
@@ -216,7 +215,7 @@ static int get_or_create_session(struct xlation *state, struct bib_entry *bib,
 	if (error)
 		return error;
 
-	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL);
+	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL, 1);
 	if (error) {
 		session_put(session, true);
 		return error;
@@ -367,14 +366,15 @@ static int tcp_closed_v6_syn(struct xlation *state)
 		goto end;
 	session->state = V6_INIT;
 
-	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL);
+	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL, 0);
 	if (error) {
 		session_put(session, true);
 		goto end;
 	}
-
 	log_session(session);
+
 	joold_add(state, session);
+
 	/* Transfer session refcount to @state; do not put yet. */
 	state->session = session;
 	/* Fall through. */
@@ -427,13 +427,14 @@ static verdict tcp_closed_v4_syn(struct xlation *state)
 		goto end_session;
 	}
 
-	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL);
+	error = sessiondb_add(state->jool.nat64.session, session, NULL, NULL, 0);
 	if (error) {
 		log_debug("Could not add session to database: %d", error);
 		goto end_session;
 	}
 
 	joold_add(state, session);
+
 	/* Transfer session refcount to @state; do not put yet. */
 	state->session = session;
 	bibentry_put_thread(bib, false);
@@ -504,7 +505,6 @@ static enum session_fate tcp_v4_init_state(struct session_entry *session,
 
 	if (pkt_l3_proto(pkt) == L3PROTO_IPV6 && pkt_tcp_hdr(pkt)->syn) {
 		session->state = ESTABLISHED;
-		joold_add(state, session);
 		return FATE_TIMER_EST;
 	}
 
@@ -524,7 +524,6 @@ static enum session_fate tcp_v6_init_state(struct session_entry *session,
 		switch (pkt_l3_proto(pkt)) {
 		case L3PROTO_IPV4:
 			session->state = ESTABLISHED;
-			joold_add(state, session);
 			return FATE_TIMER_EST;
 		case L3PROTO_IPV6:
 			return FATE_TIMER_TRANS;
@@ -547,11 +546,9 @@ static enum session_fate tcp_established_state(struct session_entry *session,
 		switch (pkt_l3_proto(pkt)) {
 		case L3PROTO_IPV4:
 			session->state = V4_FIN_RCV;
-			joold_add(state, session);
 			break;
 		case L3PROTO_IPV6:
 			session->state = V6_FIN_RCV;
-			joold_add(state, session);
 			break;
 		}
 		return FATE_PRESERVE;
@@ -575,7 +572,6 @@ static enum session_fate tcp_v4_fin_rcv_state(struct session_entry *session,
 
 	if (pkt_l3_proto(pkt) == L3PROTO_IPV6 && pkt_tcp_hdr(pkt)->fin) {
 		session->state = V4_FIN_V6_FIN_RCV;
-		joold_add(state, session);
 		return FATE_TIMER_TRANS;
 	}
 
@@ -593,7 +589,6 @@ static enum session_fate tcp_v6_fin_rcv_state(struct session_entry *session,
 
 	if (pkt_l3_proto(pkt) == L3PROTO_IPV4 && pkt_tcp_hdr(pkt)->fin) {
 		session->state = V4_FIN_V6_FIN_RCV;
-		joold_add(state, session);
 		return FATE_TIMER_TRANS;
 	}
 
@@ -621,7 +616,6 @@ static enum session_fate tcp_trans_state(struct session_entry *session,
 
 	if (!pkt_tcp_hdr(pkt)->rst) {
 		session->state = ESTABLISHED;
-		joold_add(state, session);
 		return FATE_TIMER_EST;
 	}
 
@@ -679,8 +673,31 @@ static verdict tcp(struct xlation *state)
 		inc_stats(&state->in, IPSTATS_MIB_INDISCARDS);
 		return VERDICT_DROP;
 	}
-
 	log_session(session);
+
+	/*
+	 * Sometimes the session doesn't change as a result of the state
+	 * machine's schemes.
+	 * No state change, no timeout change, no update time change.
+	 *
+	 * One might argue that we shouldn't joold the session in those cases.
+	 * It's a lot more trouble than it's worth:
+	 *
+	 * - Calling joold_add() on the TCP SM state functions is incorrect
+	 *   because the session's update_time and expirer haven't been updated
+	 *   by that point. So what gets synchronizes is half-baked data.
+	 * - Calling joold_add() on decide_fate() is a freaking mess because
+	 *   we'd need to send the xlator and a boolean (indicating whether this
+	 *   is packet or timer context) to it and all intermediate functions,
+	 *   and these functions all already have too many arguments as it is.
+	 *   It's bad design anyway; the session module belongs to a layer that
+	 *   shouldn't be aware of the xlator.
+	 * - These special no-changes cases are rare.
+	 *
+	 * So let's simplify everything by just joold_add()ing here.
+	 */
+	joold_add(state, session);
+
 	/* Transfer session refcount to @state; do not put yet. */
 	state->session = session;
 	return VERDICT_CONTINUE;

mod/stateful/joold.c

@@ -1,7 +1,6 @@
 #include "nat64/mod/stateful/joold.h"
 
 #include "nat64/common/constants.h"
-#include "nat64/common/session.h"
 #include "nat64/common/str_utils.h"
 #include "nat64/mod/common/wkmalloc.h"
 #include "nat64/common/joold/joold_config.h"
@@ -82,6 +81,7 @@ struct joold_session {
 	struct ipv6_transport_addr dst6;
 	struct ipv4_transport_addr src4;
 	struct ipv4_transport_addr dst4;
+	__u8 l4_proto;
 	/**
 	 * This is not actually the same as session_entry->update_time.
 	 * session_entry->update_time is the time at which the session was last
@@ -95,10 +95,12 @@ struct joold_session {
 	 * This one is measured in milliseconds.
 	 */
 	__be64 update_time;
-	/** The quirks of @update_time also apply here. */
-	__be64 creation_time;
-	__u8 l4_proto;
 	__u8 state;
+	/*
+	 * true: timer is established.
+	 * false: timer is transitory.
+	 */
+	__u8 est;
 };
 
 /**
@@ -198,17 +200,23 @@ static bool should_send(struct joold_queue *queue)
 static int write_single_node(struct joold_node *node,
 		struct nlcore_buffer *buffer)
 {
+	__be64 old;
 	__u64 time;
+	int full;
+
+	old = node->single.update_time;
 
 	time = be64_to_cpu(node->single.update_time);
 	time = jiffies_to_msecs(jiffies - time);
 	node->single.update_time = cpu_to_be64(time);
 
-	time = be64_to_cpu(node->single.creation_time);
-	time = jiffies_to_msecs(jiffies - time);
-	node->single.creation_time = cpu_to_be64(time);
+	full = nlbuffer_write(buffer, &node->single, sizeof(node->single));
+	if (full) {
+		/* We'll convert the time again in the next flush, so revert. */
+		node->single.update_time = old;
+	}
 
-	return nlbuffer_write(buffer, &node->single, sizeof(node->single));
+	return full;
 }
 
 static int foreach_cb(struct session_entry *entry, void *arg)
@@ -217,20 +225,16 @@ static int foreach_cb(struct session_entry *entry, void *arg)
 	struct joold_advertise_struct *adv = arg;
 	struct joold_session session;
 	__u64 update_time;
-	__u64 creation_time;
 
-	session.l4_proto = entry->l4_proto;
 	session.src4 = entry->src4;
 	session.dst6 = entry->dst6;
 	session.dst4 = entry->dst4;
 	session.src6 = entry->src6;
-	session.state = entry->state;
-
+	session.l4_proto = entry->l4_proto;
 	update_time = jiffies_to_msecs(jiffies - entry->update_time);
 	session.update_time = cpu_to_be64(update_time);
-
-	creation_time = jiffies_to_msecs(jiffies - entry->creation_time);
-	session.creation_time = cpu_to_be64(creation_time);
+	session.state = entry->state;
+	session.est = entry->expirer->is_established;
 
 	status = nlbuffer_write(adv->buffer, &session, sizeof(session));
 	if (status) {
@@ -310,7 +314,6 @@ static int build_buffer(struct nlcore_buffer *buffer, struct joold_queue *queue,
 	}
 
 	return 0;
-
 }
 
 /*
@@ -464,18 +467,18 @@ void joold_add_session(struct joold_queue *queue, struct session_entry *entry,
 	}
 
 	copy->is_group = false;
-	copy->single.l4_proto = entry->l4_proto;
-	copy->single.src4 = entry->src4;
+	copy->single.src6 = entry->src6;
 	copy->single.dst6 = entry->dst6;
+	copy->single.src4 = entry->src4;
 	copy->single.dst4 = entry->dst4;
-	copy->single.src6 = entry->src6;
-	copy->single.state = entry->state;
 	/*
-	 * Do not convert the times yet; if the session is queued for a long
+	 * Do not convert the time yet; if the session is queued for a long
 	 * time, these will be horribly inaccurate.
 	 */
 	copy->single.update_time = cpu_to_be64(entry->update_time);
-	copy->single.creation_time = cpu_to_be64(entry->creation_time);
+	copy->single.l4_proto = entry->l4_proto;
+	copy->single.state = entry->state;
+	copy->single.est = entry->expirer->is_established;
 
 	list_add_tail(&copy->nextprev, &queue->sessions);
 	queue->count++;
@@ -553,7 +556,6 @@ static struct session_entry *init_session_entry(struct joold_session *in,
 {
 	struct session_entry *out;
 	__u64 update_time;
-	__u64 creation_time;
 
 	out = session_create(&in->src6, &in->dst6, &in->src4,
 			&in->dst4, in->l4_proto, bib);
@@ -562,12 +564,9 @@ static struct session_entry *init_session_entry(struct joold_session *in,
 
 	update_time = be64_to_cpu(in->update_time);
 	update_time = jiffies - msecs_to_jiffies(update_time);
-	creation_time = be64_to_cpu(in->creation_time);
-	creation_time = jiffies - msecs_to_jiffies(creation_time);
 
 	out->state = in->state;
 	out->update_time = update_time;
-	out->creation_time = creation_time;
 
 	return out;
 }
@@ -626,7 +625,9 @@ static bool add_new_session(struct xlator *jool, struct joold_session *in)
 	}
 
 	params.new = new;
-	error = sessiondb_add(jool->nat64.session, new, collision_cb, &params);
+	error = sessiondb_add(jool->nat64.session, new,
+			collision_cb, &params,
+			in->est);
 	if (error == -EEXIST) {
 		session_put(new, true);
 		return params.success;
@@ -665,8 +666,8 @@ static int validate_enabled(struct xlator *jool)
 }
 
 /**
- * joold_sync_entries - Parses a bunch of sessions out of @data and adds them
- * to @jool's session database.
+ * joold_sync - Parses a bunch of sessions out of @data and adds them to @jool's
+ * session database.
  *
  * This is the function that gets called whenever the jool daemon sends data to
  * the @jool Jool instance.

mod/stateful/session/db.c

@@ -120,14 +120,14 @@ bool sessiondb_allow(struct sessiondb *db, struct tuple *tuple4)
 }
 
 int sessiondb_add(struct sessiondb *db, struct session_entry *session,
-		fate_cb cb, void *cb_args)
+		fate_cb cb, void *cb_args, bool est_timer)
 {
 	struct session_table *table = get_table(db, session->l4_proto);
 	if (!table)
 		return -EINVAL;
 
 	pktqueue_rm(db->pkt_queue, session);
-	return sessiontable_add(table, session, cb, cb_args);
+	return sessiontable_add(table, session, cb, cb_args, est_timer);
 }
 
 int sessiondb_set_session_timer(struct sessiondb *db, struct session_entry *session, bool is_established)

mod/stateful/session/entry.c

@@ -39,7 +39,6 @@ struct session_entry *session_create(const struct ipv6_transport_addr *src6,
 			.l4_proto = l4_proto,
 			.state = 0,
 			.update_time = jiffies,
-			.creation_time = jiffies,
 			.bib = bib,
 			.expirer = NULL,
 	};