* [PATCH 0/3] l2tp: make datapath sequence number handling RFC-compliant
@ 2013-07-02 10:00 James Chapman
2013-07-02 10:00 ` [PATCH 1/3] l2tp: do data sequence number handling in a separate func James Chapman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: James Chapman @ 2013-07-02 10:00 UTC (permalink / raw)
To: netdev; +Cc: 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.
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 | 116 +++++++++++++++++++++++++++++++++++++++----------
net/l2tp/l2tp_core.h | 5 ++
2 files changed, 97 insertions(+), 24 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] l2tp: do data sequence number handling in a separate func
2013-07-02 10:00 [PATCH 0/3] l2tp: make datapath sequence number handling RFC-compliant James Chapman
@ 2013-07-02 10:00 ` James Chapman
2013-07-02 16:28 ` Sergei Shtylyov
2013-07-02 10:00 ` [PATCH 2/3] l2tp: make datapath sequence number support RFC-compliant James Chapman
2013-07-02 10:00 ` [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled James Chapman
2 siblings, 1 reply; 6+ messages in thread
From: James Chapman @ 2013-07-02 10:00 UTC (permalink / raw)
To: netdev; +Cc: 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 | 55 +++++++++++++++++++++++++++++++------------------
1 files changed, 35 insertions(+), 20 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6984c3a..b2389fe 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -542,6 +542,39 @@ 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 +790,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] l2tp: make datapath sequence number support RFC-compliant
2013-07-02 10:00 [PATCH 0/3] l2tp: make datapath sequence number handling RFC-compliant James Chapman
2013-07-02 10:00 ` [PATCH 1/3] l2tp: do data sequence number handling in a separate func James Chapman
@ 2013-07-02 10:00 ` James Chapman
2013-07-02 10:00 ` [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled James Chapman
2 siblings, 0 replies; 6+ messages in thread
From: James Chapman @ 2013-07-02 10:00 UTC (permalink / raw)
To: netdev; +Cc: 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 b2389fe..cc7ece9 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, "
@@ -1827,6 +1848,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled
2013-07-02 10:00 [PATCH 0/3] l2tp: make datapath sequence number handling RFC-compliant James Chapman
2013-07-02 10:00 ` [PATCH 1/3] l2tp: do data sequence number handling in a separate func James Chapman
2013-07-02 10:00 ` [PATCH 2/3] l2tp: make datapath sequence number support RFC-compliant James Chapman
@ 2013-07-02 10:00 ` James Chapman
2013-07-02 16:32 ` Sergei Shtylyov
2 siblings, 1 reply; 6+ messages in thread
From: James Chapman @ 2013-07-02 10:00 UTC (permalink / raw)
To: netdev; +Cc: 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 | 37 ++++++++++++++++++++++++++++++++-----
net/l2tp/l2tp_core.h | 3 +++
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index cc7ece9..f572e93 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -572,12 +572,34 @@ 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, "
@@ -590,6 +612,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:
@@ -1853,6 +1876,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] l2tp: do data sequence number handling in a separate func
2013-07-02 10:00 ` [PATCH 1/3] l2tp: do data sequence number handling in a separate func James Chapman
@ 2013-07-02 16:28 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2013-07-02 16:28 UTC (permalink / raw)
To: James Chapman; +Cc: netdev
Hello.
On 02-07-2013 14:00, James Chapman wrote:
> 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 | 55 +++++++++++++++++++++++++++++++------------------
> 1 files changed, 35 insertions(+), 20 deletions(-)
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 6984c3a..b2389fe 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -542,6 +542,39 @@ 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",
You shouldn't break the long message strings -- leave it as it was
please.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled
2013-07-02 10:00 ` [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled James Chapman
@ 2013-07-02 16:32 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2013-07-02 16:32 UTC (permalink / raw)
To: James Chapman; +Cc: netdev
Hello.
On 02-07-2013 14:00, James Chapman wrote:
> 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 | 37 ++++++++++++++++++++++++++++++++-----
> net/l2tp/l2tp_core.h | 3 +++
> 2 files changed, 35 insertions(+), 5 deletions(-)
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index cc7ece9..f572e93 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -572,12 +572,34 @@ static int l2tp_recv_data_seq(struct l2tp_session *session, struct sk_buff *skb)
[...]
> + 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",
Same comment about the long message string. Not breaking such
strings facilitates grepping in the kernel for them. In theory,
checkpatch.pl should have complained here, but it might not have
recognized the function. If it complains about long string, just ignore it.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-02 16:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-02 10:00 [PATCH 0/3] l2tp: make datapath sequence number handling RFC-compliant James Chapman
2013-07-02 10:00 ` [PATCH 1/3] l2tp: do data sequence number handling in a separate func James Chapman
2013-07-02 16:28 ` Sergei Shtylyov
2013-07-02 10:00 ` [PATCH 2/3] l2tp: make datapath sequence number support RFC-compliant James Chapman
2013-07-02 10:00 ` [PATCH 3/3] l2tp: make datapath resilient to packet loss when sequence numbers enabled James Chapman
2013-07-02 16:32 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).