[PATCH v2 0/3] l2tp: make datapath sequence number handling RFC-compliant

[PATCH v2 0/3] l2tp: make datapath sequence number handling RFC-compliant

[James Chapman]

L2TP data sequence numbers, if enabled, ensure in-order delivery. A
receiver may reorder data packets, or simply drop out-of-sequence
packets. If reordering is not enabled, the current implementation does
not handle data packet loss correctly, which can result in a stalled
L2TP session datapath as soon as the first packet is lost. Most L2TP
users either disable sequence numbers or enable data packet reordering
when sequence numbers are used to circumvent the issue. This patch
series fixes the problem, and makes the L2TP sequence number handling
RFC-compliant.

v2 incorporates string format changes requested by sergei.shtylyov@cogentembedded.com.

James Chapman (3):
  l2tp: do data sequence number handling in a separate func
  l2tp: make datapath sequence number support RFC-compliant
  l2tp: make datapath resilient to packet loss when sequence numbers
    enabled

 net/l2tp/l2tp_core.c |  114 +++++++++++++++++++++++++++++++++++++++----------
 net/l2tp/l2tp_core.h |    5 ++
 2 files changed, 95 insertions(+), 24 deletions(-)

[PATCH v2 1/3] l2tp: do data sequence number handling in a separate func

[James Chapman]

This change moves some code handling data sequence numbers into a
separate function to avoid too much indentation. This is to prepare
for some changes to data sequence number handling in subsequent
patches.

Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |   54 +++++++++++++++++++++++++++++++------------------
 1 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6984c3a..5ca2965 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -542,6 +542,38 @@ static inline int l2tp_verify_udp_checksum(struct sock *sk,
 	return __skb_checksum_complete(skb);
 }
 
+/* If packet has sequence numbers, queue it if acceptable. Returns 0 if
+ * acceptable, else non-zero.
+ */
+static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
+{
+	if (session->reorder_timeout != 0) {
+		/* Packet reordering enabled. Add skb to session's
+		 * reorder queue, in order of ns.
+		 */
+		l2tp_recv_queue_skb(session, skb);
+	} else {
+		/* Packet reordering disabled. Discard out-of-sequence
+		 * packets
+		 */
+		if (L2TP_SKB_CB(skb)->ns != session->nr) {
+			atomic_long_inc(&session->stats.rx_seq_discards);
+			l2tp_dbg(session, L2TP_MSG_SEQ,
+				 "%s: oos pkt %u len %d discarded, waiting for %u, reorder_q_len=%d\n",
+				 session->name, L2TP_SKB_CB(skb)->ns,
+				 L2TP_SKB_CB(skb)->length, session->nr,
+				 skb_queue_len(&session->reorder_q));
+			goto discard;
+		}
+		skb_queue_tail(&session->reorder_q, skb);
+	}
+
+	return 0;
+
+discard:
+	return 1;
+}
+
 /* Do receive processing of L2TP data frames. We handle both L2TPv2
  * and L2TPv3 data frames here.
  *
@@ -757,26 +789,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	 * enabled. Saved L2TP protocol info is stored in skb->sb[].
 	 */
 	if (L2TP_SKB_CB(skb)->has_seq) {
-		if (session->reorder_timeout != 0) {
-			/* Packet reordering enabled. Add skb to session's
-			 * reorder queue, in order of ns.
-			 */
-			l2tp_recv_queue_skb(session, skb);
-		} else {
-			/* Packet reordering disabled. Discard out-of-sequence
-			 * packets
-			 */
-			if (L2TP_SKB_CB(skb)->ns != session->nr) {
-				atomic_long_inc(&session->stats.rx_seq_discards);
-				l2tp_dbg(session, L2TP_MSG_SEQ,
-					 "%s: oos pkt %u len %d discarded, waiting for %u, reorder_q_len=%d\n",
-					 session->name, L2TP_SKB_CB(skb)->ns,
-					 L2TP_SKB_CB(skb)->length, session->nr,
-					 skb_queue_len(&session->reorder_q));
-				goto discard;
-			}
-			skb_queue_tail(&session->reorder_q, skb);
-		}
+		if (l2tp_recv_data_seq(session, skb))
+			goto discard;
 	} else {
 		/* No sequence numbers. Add the skb to the tail of the
 		 * reorder queue. This ensures that it will be
-- 
1.7.0.4

[PATCH v2 2/3] l2tp: make datapath sequence number support RFC-compliant

[James Chapman]

The L2TP datapath is not currently RFC-compliant when sequence numbers
are used in L2TP data packets. According to the L2TP RFC, any received
sequence number NR greater than or equal to the next expected NR is
acceptable, where the "greater than or equal to" test is determined by
the NR wrap point. This differs for L2TPv2 and L2TPv3, so add state in
the session context to hold the max NR value and the NR window size in
order to do the acceptable sequence number value check. These might be
configurable later, but for now we derive it from the tunnel L2TP
version, which determines the sequence number field size.

Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |   36 +++++++++++++++++++++++++++++++-----
 net/l2tp/l2tp_core.h |    2 ++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 5ca2965..735cc06 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -414,10 +414,7 @@ static void l2tp_recv_dequeue_skb(struct l2tp_session *session, struct sk_buff *
 	if (L2TP_SKB_CB(skb)->has_seq) {
 		/* Bump our Nr */
 		session->nr++;
-		if (tunnel->version == L2TP_HDR_VER_2)
-			session->nr &= 0xffff;
-		else
-			session->nr &= 0xffffff;
+		session->nr &= session->nr_max;
 
 		l2tp_dbg(session, L2TP_MSG_SEQ, "%s: updated nr to %hu\n",
 			 session->name, session->nr);
@@ -542,11 +539,34 @@ static inline int l2tp_verify_udp_checksum(struct sock *sk,
 	return __skb_checksum_complete(skb);
 }
 
+static int l2tp_seq_check_rx_window(struct l2tp_session *session, u32 nr)
+{
+	u32 nws;
+
+	if (nr >= session->nr)
+		nws = nr - session->nr;
+	else
+		nws = (session->nr_max + 1) - (session->nr - nr);
+
+	return nws < session->nr_window_size;
+}
+
 /* If packet has sequence numbers, queue it if acceptable. Returns 0 if
  * acceptable, else non-zero.
  */
 static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
 {
+	if (!l2tp_seq_check_rx_window(session, L2TP_SKB_CB(skb)->ns)) {
+		/* Packet sequence number is outside allowed window.
+		 * Discard it.
+		 */
+		l2tp_dbg(session, L2TP_MSG_SEQ,
+			 "%s: pkt %u len %d discarded, outside window, nr=%u\n",
+			 session->name, L2TP_SKB_CB(skb)->ns,
+			 L2TP_SKB_CB(skb)->length, session->nr);
+		goto discard;
+	}
+
 	if (session->reorder_timeout != 0) {
 		/* Packet reordering enabled. Add skb to session's
 		 * reorder queue, in order of ns.
@@ -556,7 +576,8 @@ static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
 		/* Packet reordering disabled. Discard out-of-sequence
 		 * packets
 		 */
-		if (L2TP_SKB_CB(skb)->ns != session->nr) {
+		if ((L2TP_SKB_CB(skb)->ns != session->nr) &&
+		    (!session->reorder_skip)) {
 			atomic_long_inc(&session->stats.rx_seq_discards);
 			l2tp_dbg(session, L2TP_MSG_SEQ,
 				 "%s: oos pkt %u len %d discarded, waiting for %u, reorder_q_len=%d\n",
@@ -1826,6 +1847,11 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 		session->session_id = session_id;
 		session->peer_session_id = peer_session_id;
 		session->nr = 0;
+		if (tunnel->version == L2TP_HDR_VER_2)
+			session->nr_max = 0xffff;
+		else
+			session->nr_max = 0xffffff;
+		session->nr_window_size = session->nr_max / 2;
 
 		sprintf(&session->name[0], "sess %u/%u",
 			tunnel->tunnel_id, session->session_id);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 485a490..4b9a3b7 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -102,6 +102,8 @@ struct l2tp_session {
 	u32			nr;		/* session NR state (receive) */
 	u32			ns;		/* session NR state (send) */
 	struct sk_buff_head	reorder_q;	/* receive reorder queue */
+	u32			nr_max;		/* max NR. Depends on tunnel */
+	u32			nr_window_size;	/* NR window size */
 	struct hlist_node	hlist;		/* Hash list node */
 	atomic_t		ref_count;
 
-- 
1.7.0.4

[PATCH v2 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled

[James Chapman]

If L2TP data sequence numbers are enabled and reordering is not
enabled, data reception stops if a packet is lost since the kernel
waits for a sequence number that is never resent. (When reordering is
enabled, data reception restarts when the reorder timeout expires.) If
no reorder timeout is set, we should count the number of in-sequence
packets after the out-of-sequence (OOS) condition is detected, and reset
sequence number state after a number of such packets are received.

For now, the number of in-sequence packets while in OOS state which
cause the sequence number state to be reset is hard-coded to 5. This
could be configurable later.

Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |   36 +++++++++++++++++++++++++++++++-----
 net/l2tp/l2tp_core.h |    3 +++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 735cc06..feae495 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -572,12 +572,33 @@ static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
 		 * reorder queue, in order of ns.
 		 */
 		l2tp_recv_queue_skb(session, skb);
+		goto out;
+	}
+
+	/* Packet reordering disabled. Discard out-of-sequence packets, while
+	 * tracking the number if in-sequence packets after the first OOS packet
+	 * is seen. After nr_oos_count_max in-sequence packets, reset the
+	 * sequence number to re-enable packet reception.
+	 */
+	if (L2TP_SKB_CB(skb)->ns == session->nr) {
+		skb_queue_tail(&session->reorder_q, skb);
 	} else {
-		/* Packet reordering disabled. Discard out-of-sequence
-		 * packets
-		 */
-		if ((L2TP_SKB_CB(skb)->ns != session->nr) &&
-		    (!session->reorder_skip)) {
+		u32 nr_oos = L2TP_SKB_CB(skb)->ns;
+		u32 nr_next = (session->nr_oos + 1) & session->nr_max;
+
+		if (nr_oos == nr_next)
+			session->nr_oos_count++;
+		else
+			session->nr_oos_count = 0;
+
+		session->nr_oos = nr_oos;
+		if (session->nr_oos_count > session->nr_oos_count_max) {
+			session->reorder_skip = 1;
+			l2tp_dbg(session, L2TP_MSG_SEQ,
+				 "%s: %d oos packets received. Resetting sequence numbers\n",
+				 session->name, session->nr_oos_count);
+		}
+		if (!session->reorder_skip) {
 			atomic_long_inc(&session->stats.rx_seq_discards);
 			l2tp_dbg(session, L2TP_MSG_SEQ,
 				 "%s: oos pkt %u len %d discarded, waiting for %u, reorder_q_len=%d\n",
@@ -589,6 +610,7 @@ static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
 		skb_queue_tail(&session->reorder_q, skb);
 	}
 
+out:
 	return 0;
 
 discard:
@@ -1852,6 +1874,10 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 		else
 			session->nr_max = 0xffffff;
 		session->nr_window_size = session->nr_max / 2;
+		session->nr_oos_count_max = 4;
+
+		/* Use NR of first received packet */
+		session->reorder_skip = 1;
 
 		sprintf(&session->name[0], "sess %u/%u",
 			tunnel->tunnel_id, session->session_id);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 4b9a3b7..66a559b 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -104,6 +104,9 @@ struct l2tp_session {
 	struct sk_buff_head	reorder_q;	/* receive reorder queue */
 	u32			nr_max;		/* max NR. Depends on tunnel */
 	u32			nr_window_size;	/* NR window size */
+	u32			nr_oos;		/* NR of last OOS packet */
+	int			nr_oos_count;	/* For OOS recovery */
+	int			nr_oos_count_max;
 	struct hlist_node	hlist;		/* Hash list node */
 	atomic_t		ref_count;
 
-- 
1.7.0.4

Re: [PATCH v2 0/3] l2tp: make datapath sequence number handling RFC-compliant

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Tue,  2 Jul 2013 20:28:57 +0100

> L2TP data sequence numbers, if enabled, ensure in-order delivery. A
> receiver may reorder data packets, or simply drop out-of-sequence
> packets. If reordering is not enabled, the current implementation does
> not handle data packet loss correctly, which can result in a stalled
> L2TP session datapath as soon as the first packet is lost. Most L2TP
> users either disable sequence numbers or enable data packet reordering
> when sequence numbers are used to circumvent the issue. This patch
> series fixes the problem, and makes the L2TP sequence number handling
> RFC-compliant.
> 
> v2 incorporates string format changes requested by sergei.shtylyov@cogentembedded.com.

Series applied, thanks James.