[PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

This series of patches resolves several SCTP association hangs observed during
SCTP stress testing.  Observable symptoms include communications hangs with
data being held in the association reassembly and/or lobby (ordering) queues.
Close examination of reassembly/ordering queues may show either duplicated
or missing packets.

Lee A. Roberts (4):
  sctp: fix association hangs due to off-by-one errors in
    sctp_tsnmap_grow()
  sctp: fix association hangs due to reneging packets below the
    cumulative TSN ACK point
  sctp: fix association hangs due to errors when reneging events from
    the ordering queue
  sctp: fix association hangs due to partial delivery errors

 net/sctp/tsnmap.c   |   13 ++++----
 net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 78 insertions(+), 22 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association lobby (ordering)
queue.  Close examination of reassembly/ordering queues shows
duplicated packets.

In sctp_tsnmap_mark(), correct off-by-one error when calculating
size value for sctp_tsnmap_grow().

In sctp_tsnmap_grow(), correct off-by-one error when copying
and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
word, this bit can be lost, causing the corresponding packet
to be transmitted again and to be entered as a duplicate into
the SCTP reassembly/ordering queues.  Change parameter name
from "gap" (zero-based index) to "size" (one-based) to enhance
code readability.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/tsnmap.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sctp/tsnmap.c b/net/sctp/tsnmap.c
index 5f25e0c..396c451 100644
--- a/net/sctp/tsnmap.c
+++ b/net/sctp/tsnmap.c
@@ -51,7 +51,7 @@
 static void sctp_tsnmap_update(struct sctp_tsnmap *map);
 static void sctp_tsnmap_find_gap_ack(unsigned long *map, __u16 off,
 				     __u16 len, __u16 *start, __u16 *end);
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap);
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size);
 
 /* Initialize a block of memory as a tsnmap.  */
 struct sctp_tsnmap *sctp_tsnmap_init(struct sctp_tsnmap *map, __u16 len,
@@ -124,7 +124,7 @@ int sctp_tsnmap_mark(struct sctp_tsnmap *map, __u32 tsn,
 
 	gap = tsn - map->base_tsn;
 
-	if (gap >= map->len && !sctp_tsnmap_grow(map, gap))
+	if (gap >= map->len && !sctp_tsnmap_grow(map, gap + 1))
 		return -ENOMEM;
 
 	if (!sctp_tsnmap_has_gap(map) && gap == 0) {
@@ -360,23 +360,24 @@ __u16 sctp_tsnmap_num_gabs(struct sctp_tsnmap *map,
 	return ngaps;
 }
 
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap)
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size)
 {
 	unsigned long *new;
 	unsigned long inc;
 	u16  len;
 
-	if (gap >= SCTP_TSN_MAP_SIZE)
+	if (size > SCTP_TSN_MAP_SIZE)
 		return 0;
 
-	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
+	inc = ALIGN((size - map->len), BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
 	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
 
 	new = kzalloc(len>>3, GFP_ATOMIC);
 	if (!new)
 		return 0;
 
-	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
+	bitmap_copy(new, map->tsn_map,
+		map->max_tsn_seen - map->cumulative_tsn_ack_point);
 	kfree(map->tsn_map);
 	map->tsn_map = new;
 	map->len = len;
-- 
1.7.9.5

Re: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Vlad Yasevich]

On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association lobby (ordering)
> queue.  Close examination of reassembly/ordering queues shows
> duplicated packets.
>
> In sctp_tsnmap_mark(), correct off-by-one error when calculating
> size value for sctp_tsnmap_grow().
>
> In sctp_tsnmap_grow(), correct off-by-one error when copying
> and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
> word, this bit can be lost, causing the corresponding packet
> to be transmitted again and to be entered as a duplicate into
> the SCTP reassembly/ordering queues.  Change parameter name
> from "gap" (zero-based index) to "size" (one-based) to enhance
> code readability.
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/tsnmap.c |   13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/sctp/tsnmap.c b/net/sctp/tsnmap.c
> index 5f25e0c..396c451 100644
> --- a/net/sctp/tsnmap.c
> +++ b/net/sctp/tsnmap.c
> @@ -51,7 +51,7 @@
>   static void sctp_tsnmap_update(struct sctp_tsnmap *map);
>   static void sctp_tsnmap_find_gap_ack(unsigned long *map, __u16 off,
>   				     __u16 len, __u16 *start, __u16 *end);
> -static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap);
> +static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size);
>
>   /* Initialize a block of memory as a tsnmap.  */
>   struct sctp_tsnmap *sctp_tsnmap_init(struct sctp_tsnmap *map, __u16 len,
> @@ -124,7 +124,7 @@ int sctp_tsnmap_mark(struct sctp_tsnmap *map, __u32 tsn,
>
>   	gap = tsn - map->base_tsn;
>
> -	if (gap >= map->len && !sctp_tsnmap_grow(map, gap))
> +	if (gap >= map->len && !sctp_tsnmap_grow(map, gap + 1))
>   		return -ENOMEM;
>
>   	if (!sctp_tsnmap_has_gap(map) && gap == 0) {
> @@ -360,23 +360,24 @@ __u16 sctp_tsnmap_num_gabs(struct sctp_tsnmap *map,
>   	return ngaps;
>   }
>
> -static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap)
> +static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size)
>   {
>   	unsigned long *new;
>   	unsigned long inc;
>   	u16  len;
>
> -	if (gap >= SCTP_TSN_MAP_SIZE)
> +	if (size > SCTP_TSN_MAP_SIZE)
>   		return 0;
>
> -	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
> +	inc = ALIGN((size - map->len), BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
>   	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
>
>   	new = kzalloc(len>>3, GFP_ATOMIC);
>   	if (!new)
>   		return 0;
>
> -	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
> +	bitmap_copy(new, map->tsn_map,
> +		map->max_tsn_seen - map->cumulative_tsn_ack_point);
>   	kfree(map->tsn_map);
>   	map->tsn_map = new;
>   	map->len = len;
>

[PATCH 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_renege_list(), do not renege packets below the
cumulative TSN ACK point.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index ada1746..63afddc 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -969,11 +969,16 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 
 	tsnmap = &ulpq->asoc->peer.tsn_map;
 
-	while ((skb = __skb_dequeue_tail(list)) != NULL) {
-		freed += skb_headlen(skb);
+	while ((skb = skb_peek_tail(list)) != NULL) {
 		event = sctp_skb2event(skb);
 		tsn = event->tsn;
 
+		/* Don't renege below the Cumulative TSN ACK Point. */
+		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
+			break;
+
+		__skb_unlink(skb, list);
+		freed += skb_headlen(skb);
 		sctp_ulpevent_free(event);
 		sctp_tsnmap_renege(tsnmap, tsn);
 		if (freed >= needed)
-- 
1.7.9.5

Re: [PATCH 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point

[Vlad Yasevich]

On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
>
> In sctp_ulpq_renege_list(), do not renege packets below the
> cumulative TSN ACK point.
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/ulpqueue.c |    9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index ada1746..63afddc 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -969,11 +969,16 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
>
>   	tsnmap = &ulpq->asoc->peer.tsn_map;
>
> -	while ((skb = __skb_dequeue_tail(list)) != NULL) {
> -		freed += skb_headlen(skb);
> +	while ((skb = skb_peek_tail(list)) != NULL) {
>   		event = sctp_skb2event(skb);
>   		tsn = event->tsn;
>
> +		/* Don't renege below the Cumulative TSN ACK Point. */
> +		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
> +			break;
> +
> +		__skb_unlink(skb, list);
> +		freed += skb_headlen(skb);
>   		sctp_ulpevent_free(event);
>   		sctp_tsnmap_renege(tsnmap, tsn);
>   		if (freed >= needed)
>

[PATCH 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_renege_list(), events being reneged from the
ordering queue may correspond to multiple TSNs.  Identify
all affected packets; sum freed space and renege from the
tsnmap.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 63afddc..f221fbb 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -962,8 +962,8 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		struct sk_buff_head *list, __u16 needed)
 {
 	__u16 freed = 0;
-	__u32 tsn;
-	struct sk_buff *skb;
+	__u32 tsn, last_tsn;
+	struct sk_buff *skb, *flist, *last;
 	struct sctp_ulpevent *event;
 	struct sctp_tsnmap *tsnmap;
 
@@ -977,10 +977,28 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
 			break;
 
-		__skb_unlink(skb, list);
+		/* Events in ordering queue may have multiple fragments
+		 * corresponding to additional TSNs.  Sum the total
+		 * freed space; find the last TSN.
+		 */
 		freed += skb_headlen(skb);
+		flist = skb_shinfo(skb)->frag_list;
+		for (last = flist; flist; flist = flist->next) {
+			last = flist;
+			freed += skb_headlen(last);
+		}
+		if (last)
+			last_tsn = sctp_skb2event(last)->tsn;
+		else
+			last_tsn = tsn;
+
+		/* Unlink the event, then renege all applicable TSNs. */
+		__skb_unlink(skb, list);
 		sctp_ulpevent_free(event);
-		sctp_tsnmap_renege(tsnmap, tsn);
+		while (TSN_lte(tsn, last_tsn)) {
+			sctp_tsnmap_renege(tsnmap, tsn);
+			tsn++;
+		}
 		if (freed >= needed)
 			return freed;
 	}
-- 
1.7.9.5

Re: [PATCH 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue

[Vlad Yasevich]

On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
>
> In sctp_ulpq_renege_list(), events being reneged from the
> ordering queue may correspond to multiple TSNs.  Identify
> all affected packets; sum freed space and renege from the
> tsnmap.
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/ulpqueue.c |   26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index 63afddc..f221fbb 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -962,8 +962,8 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
>   		struct sk_buff_head *list, __u16 needed)
>   {
>   	__u16 freed = 0;
> -	__u32 tsn;
> -	struct sk_buff *skb;
> +	__u32 tsn, last_tsn;
> +	struct sk_buff *skb, *flist, *last;
>   	struct sctp_ulpevent *event;
>   	struct sctp_tsnmap *tsnmap;
>
> @@ -977,10 +977,28 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
>   		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
>   			break;
>
> -		__skb_unlink(skb, list);
> +		/* Events in ordering queue may have multiple fragments
> +		 * corresponding to additional TSNs.  Sum the total
> +		 * freed space; find the last TSN.
> +		 */
>   		freed += skb_headlen(skb);
> +		flist = skb_shinfo(skb)->frag_list;
> +		for (last = flist; flist; flist = flist->next) {
> +			last = flist;
> +			freed += skb_headlen(last);
> +		}
> +		if (last)
> +			last_tsn = sctp_skb2event(last)->tsn;
> +		else
> +			last_tsn = tsn;
> +
> +		/* Unlink the event, then renege all applicable TSNs. */
> +		__skb_unlink(skb, list);
>   		sctp_ulpevent_free(event);
> -		sctp_tsnmap_renege(tsnmap, tsn);
> +		while (TSN_lte(tsn, last_tsn)) {
> +			sctp_tsnmap_renege(tsnmap, tsn);
> +			tsn++;
> +		}
>   		if (freed >= needed)
>   			return freed;
>   	}
>

[PATCH 4/4] sctp: fix association hangs due to partial delivery errors

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
a complete event (with MSG_EOR set) was delivered.  A return value
of -ENOMEM continues to indicate an out-of-memory condition was
encountered.

In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
correct message reassembly logic for SCTP partial delivery.
Change logic to ensure that as much data as possible is sent
with the initial partial delivery and that following partial
deliveries contain all available data.

In sctp_ulpq_partial_delivery(), attempt partial delivery only
if the data on the head of the reassembly queue is at or before
the cumulative TSN ACK point.

In sctp_ulpq_renege(), use the modified return values from
sctp_ulpq_tail_data() to choose whether to attempt partial
delivery or to attempt to drain the reassembly queue as a
means to reduce memory pressure.  Remove call to
sctp_tsnmap_mark(), as this is handled correctly in call to
sctp_ulpq_tail_data().

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index f221fbb..482e3ea 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 {
 	struct sk_buff_head temp;
 	struct sctp_ulpevent *event;
+	int event_eor = 0;
 
 	/* Create an event from the incoming chunk. */
 	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
@@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	/* Send event to the ULP.  'event' is the sctp_ulpevent for
 	 * very first SKB on the 'temp' list.
 	 */
-	if (event)
+	if (event) {
+		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
 		sctp_ulpq_tail_event(ulpq, event);
+	}
 
-	return 0;
+	return event_eor;
 }
 
 /* Add a new event for propagation to the ULP.  */
@@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
 		ctsn = cevent->tsn;
 
 		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
+		case SCTP_DATA_FIRST_FRAG:
+			if (!first_frag)
+				return NULL;
+			goto done;
 		case SCTP_DATA_MIDDLE_FRAG:
 			if (!first_frag) {
 				first_frag = pos;
 				next_tsn = ctsn + 1;
 				last_frag = pos;
-			} else if (next_tsn == ctsn)
+			} else if (next_tsn == ctsn) {
 				next_tsn++;
-			else
+				last_frag = pos;
+			} else
 				goto done;
 			break;
 		case SCTP_DATA_LAST_FRAG:
@@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
 			} else
 				goto done;
 			break;
+
+		case SCTP_DATA_LAST_FRAG:
+			if (!first_frag)
+				return NULL;
+			else
+				goto done;
+			break;
+
 		default:
 			return NULL;
 		}
@@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
 	struct sctp_ulpevent *event;
 	struct sctp_association *asoc;
 	struct sctp_sock *sp;
+	__u32 ctsn;
+	struct sk_buff *skb;
 
 	asoc = ulpq->asoc;
 	sp = sctp_sk(asoc->base.sk);
 
 	/* If the association is already in Partial Delivery mode
-	 * we have noting to do.
+	 * we have nothing to do.
 	 */
 	if (ulpq->pd_mode)
 		return;
 
+	/* Data must be at or below the Cumulative TSN ACK Point to
+	 * start partial delivery.
+	 */
+	skb = skb_peek(&asoc->ulpq.reasm);
+	if (skb != NULL) {
+		ctsn = sctp_skb2event(skb)->tsn;
+		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
+			return;
+	}
+
 	/* If the user enabled fragment interleave socket option,
 	 * multiple associations can enter partial delivery.
 	 * Otherwise, we can only enter partial delivery if the
@@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	}
 	/* If able to free enough room, accept this chunk. */
 	if (chunk && (freed >= needed)) {
-		__u32 tsn;
-		tsn = ntohl(chunk->subh.data_hdr->tsn);
-		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
-		sctp_ulpq_tail_data(ulpq, chunk, gfp);
-
-		sctp_ulpq_partial_delivery(ulpq, gfp);
+		int retval;
+		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
+		/*
+		 * Enter partial delivery if chunk has not been
+		 * delivered; otherwise, drain the reassembly queue.
+		 */
+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);
 	}
 
 	sk_mem_reclaim(asoc->base.sk);
-- 
1.7.9.5

Re: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors

[Vlad Yasevich]

On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
>
> In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
> a complete event (with MSG_EOR set) was delivered.  A return value
> of -ENOMEM continues to indicate an out-of-memory condition was
> encountered.
>
> In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
> correct message reassembly logic for SCTP partial delivery.
> Change logic to ensure that as much data as possible is sent
> with the initial partial delivery and that following partial
> deliveries contain all available data.
>
> In sctp_ulpq_partial_delivery(), attempt partial delivery only
> if the data on the head of the reassembly queue is at or before
> the cumulative TSN ACK point.
>
> In sctp_ulpq_renege(), use the modified return values from
> sctp_ulpq_tail_data() to choose whether to attempt partial
> delivery or to attempt to drain the reassembly queue as a
> means to reduce memory pressure.  Remove call to
> sctp_tsnmap_mark(), as this is handled correctly in call to
> sctp_ulpq_tail_data().
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
> ---
>   net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index f221fbb..482e3ea 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   {
>   	struct sk_buff_head temp;
>   	struct sctp_ulpevent *event;
> +	int event_eor = 0;
>
>   	/* Create an event from the incoming chunk. */
>   	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
> @@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   	/* Send event to the ULP.  'event' is the sctp_ulpevent for
>   	 * very first SKB on the 'temp' list.
>   	 */
> -	if (event)
> +	if (event) {
> +		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
>   		sctp_ulpq_tail_event(ulpq, event);
> +	}

You can re-use the check just before this one to catch the EOR 
condition.   You already do
	if ((event) && (event->msg_flags & MSG_EOR)){

right after you try to get a reassembled event.

Everything else looks good.
-vlad

>
> -	return 0;
> +	return event_eor;
>   }
>
>   /* Add a new event for propagation to the ULP.  */
> @@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
>   		ctsn = cevent->tsn;
>
>   		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
> +		case SCTP_DATA_FIRST_FRAG:
> +			if (!first_frag)
> +				return NULL;
> +			goto done;
>   		case SCTP_DATA_MIDDLE_FRAG:
>   			if (!first_frag) {
>   				first_frag = pos;
>   				next_tsn = ctsn + 1;
>   				last_frag = pos;
> -			} else if (next_tsn == ctsn)
> +			} else if (next_tsn == ctsn) {
>   				next_tsn++;
> -			else
> +				last_frag = pos;
> +			} else
>   				goto done;
>   			break;
>   		case SCTP_DATA_LAST_FRAG:
> @@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
>   			} else
>   				goto done;
>   			break;
> +
> +		case SCTP_DATA_LAST_FRAG:
> +			if (!first_frag)
> +				return NULL;
> +			else
> +				goto done;
> +			break;
> +
>   		default:
>   			return NULL;
>   		}
> @@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
>   	struct sctp_ulpevent *event;
>   	struct sctp_association *asoc;
>   	struct sctp_sock *sp;
> +	__u32 ctsn;
> +	struct sk_buff *skb;
>
>   	asoc = ulpq->asoc;
>   	sp = sctp_sk(asoc->base.sk);
>
>   	/* If the association is already in Partial Delivery mode
> -	 * we have noting to do.
> +	 * we have nothing to do.
>   	 */
>   	if (ulpq->pd_mode)
>   		return;
>
> +	/* Data must be at or below the Cumulative TSN ACK Point to
> +	 * start partial delivery.
> +	 */
> +	skb = skb_peek(&asoc->ulpq.reasm);
> +	if (skb != NULL) {
> +		ctsn = sctp_skb2event(skb)->tsn;
> +		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
> +			return;
> +	}
> +
>   	/* If the user enabled fragment interleave socket option,
>   	 * multiple associations can enter partial delivery.
>   	 * Otherwise, we can only enter partial delivery if the
> @@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   	}
>   	/* If able to free enough room, accept this chunk. */
>   	if (chunk && (freed >= needed)) {
> -		__u32 tsn;
> -		tsn = ntohl(chunk->subh.data_hdr->tsn);
> -		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
> -		sctp_ulpq_tail_data(ulpq, chunk, gfp);
> -
> -		sctp_ulpq_partial_delivery(ulpq, gfp);
> +		int retval;
> +		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
> +		/*
> +		 * Enter partial delivery if chunk has not been
> +		 * delivered; otherwise, drain the reassembly queue.
> +		 */
> +		if (retval <= 0)
> +			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
> +		else if (retval == 1)
> +			sctp_ulpq_reasm_drain(ulpq);
>   	}
>
>   	sk_mem_reclaim(asoc->base.sk);
>

RE: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors

[Roberts, Lee A.]

Vlad,

> -----Original Message-----
> From: Vlad Yasevich [mailto:vyasevic@redhat.com]
> Sent: Tuesday, February 26, 2013 7:35 PM
> To: Roberts, Lee A.
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors
> 
> On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> > From: "Lee A. Roberts" <lee.roberts@hp.com>
> >
> > Resolve SCTP association hangs observed during SCTP stress
> > testing.  Observable symptoms include communications hangs
> > with data being held in the association reassembly and/or lobby
> > (ordering) queues.  Close examination of reassembly queue shows
> > missing packets.
> >
> > In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
> > a complete event (with MSG_EOR set) was delivered.  A return value
> > of -ENOMEM continues to indicate an out-of-memory condition was
> > encountered.
> >
> > In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
> > correct message reassembly logic for SCTP partial delivery.
> > Change logic to ensure that as much data as possible is sent
> > with the initial partial delivery and that following partial
> > deliveries contain all available data.
> >
> > In sctp_ulpq_partial_delivery(), attempt partial delivery only
> > if the data on the head of the reassembly queue is at or before
> > the cumulative TSN ACK point.
> >
> > In sctp_ulpq_renege(), use the modified return values from
> > sctp_ulpq_tail_data() to choose whether to attempt partial
> > delivery or to attempt to drain the reassembly queue as a
> > means to reduce memory pressure.  Remove call to
> > sctp_tsnmap_mark(), as this is handled correctly in call to
> > sctp_ulpq_tail_data().
> >
> > Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
> > ---
> >   net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> > index f221fbb..482e3ea 100644
> > --- a/net/sctp/ulpqueue.c
> > +++ b/net/sctp/ulpqueue.c
> > @@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
> >   {
> >   	struct sk_buff_head temp;
> >   	struct sctp_ulpevent *event;
> > +	int event_eor = 0;
> >
> >   	/* Create an event from the incoming chunk. */
> >   	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
> > @@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
> >   	/* Send event to the ULP.  'event' is the sctp_ulpevent for
> >   	 * very first SKB on the 'temp' list.
> >   	 */
> > -	if (event)
> > +	if (event) {
> > +		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
> >   		sctp_ulpq_tail_event(ulpq, event);
> > +	}
> 
> You can re-use the check just before this one to catch the EOR
> condition.   You already do
> 	if ((event) && (event->msg_flags & MSG_EOR)){
> 
> right after you try to get a reassembled event.
> 
> Everything else looks good.
> -vlad
> 
It depends on what we want the return value to mean.  In the case where we've reneged
to be able to put in a packet, we know that the incoming packet is the next one we're
looking for.  In which case, the earlier test is correct, since this will be the next
event.

In general, the event that's being passed to sctp_ulpq_order() might not be the
next event.  In that case, sctp_ulpq_order() will call sctp_ulpq_store_ordered()
and return NULL.  Only at the last "if (event)" can we really be sure that an event
is being passed to the ULP.

In general, do we want to return 1 if a complete event is actually passed to the ULP?
Or, do we want to return 1 if we've seen a complete event come from reassembly?

                                                    - Lee

> >
> > -	return 0;
> > +	return event_eor;
> >   }
> >
> >   /* Add a new event for propagation to the ULP.  */
> > @@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq
> *ulpq)
> >   		ctsn = cevent->tsn;
> >
> >   		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
> > +		case SCTP_DATA_FIRST_FRAG:
> > +			if (!first_frag)
> > +				return NULL;
> > +			goto done;
> >   		case SCTP_DATA_MIDDLE_FRAG:
> >   			if (!first_frag) {
> >   				first_frag = pos;
> >   				next_tsn = ctsn + 1;
> >   				last_frag = pos;
> > -			} else if (next_tsn == ctsn)
> > +			} else if (next_tsn == ctsn) {
> >   				next_tsn++;
> > -			else
> > +				last_frag = pos;
> > +			} else
> >   				goto done;
> >   			break;
> >   		case SCTP_DATA_LAST_FRAG:
> > @@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
> >   			} else
> >   				goto done;
> >   			break;
> > +
> > +		case SCTP_DATA_LAST_FRAG:
> > +			if (!first_frag)
> > +				return NULL;
> > +			else
> > +				goto done;
> > +			break;
> > +
> >   		default:
> >   			return NULL;
> >   		}
> > @@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
> >   	struct sctp_ulpevent *event;
> >   	struct sctp_association *asoc;
> >   	struct sctp_sock *sp;
> > +	__u32 ctsn;
> > +	struct sk_buff *skb;
> >
> >   	asoc = ulpq->asoc;
> >   	sp = sctp_sk(asoc->base.sk);
> >
> >   	/* If the association is already in Partial Delivery mode
> > -	 * we have noting to do.
> > +	 * we have nothing to do.
> >   	 */
> >   	if (ulpq->pd_mode)
> >   		return;
> >
> > +	/* Data must be at or below the Cumulative TSN ACK Point to
> > +	 * start partial delivery.
> > +	 */
> > +	skb = skb_peek(&asoc->ulpq.reasm);
> > +	if (skb != NULL) {
> > +		ctsn = sctp_skb2event(skb)->tsn;
> > +		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
> > +			return;
> > +	}
> > +
> >   	/* If the user enabled fragment interleave socket option,
> >   	 * multiple associations can enter partial delivery.
> >   	 * Otherwise, we can only enter partial delivery if the
> > @@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
> >   	}
> >   	/* If able to free enough room, accept this chunk. */
> >   	if (chunk && (freed >= needed)) {
> > -		__u32 tsn;
> > -		tsn = ntohl(chunk->subh.data_hdr->tsn);
> > -		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
> > -		sctp_ulpq_tail_data(ulpq, chunk, gfp);
> > -
> > -		sctp_ulpq_partial_delivery(ulpq, gfp);
> > +		int retval;
> > +		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
> > +		/*
> > +		 * Enter partial delivery if chunk has not been
> > +		 * delivered; otherwise, drain the reassembly queue.
> > +		 */
> > +		if (retval <= 0)
> > +			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
> > +		else if (retval == 1)
> > +			sctp_ulpq_reasm_drain(ulpq);
> >   	}
> >
> >   	sk_mem_reclaim(asoc->base.sk);
> >

Re: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors

[Vlad Yasevich]

On 02/26/2013 11:38 PM, Roberts, Lee A. wrote:
> Vlad,
>
>> -----Original Message-----
>> From: Vlad Yasevich [mailto:vyasevic@redhat.com]
>> Sent: Tuesday, February 26, 2013 7:35 PM
>> To: Roberts, Lee A.
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors
>>
>> On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
>>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>>
>>> Resolve SCTP association hangs observed during SCTP stress
>>> testing.  Observable symptoms include communications hangs
>>> with data being held in the association reassembly and/or lobby
>>> (ordering) queues.  Close examination of reassembly queue shows
>>> missing packets.
>>>
>>> In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
>>> a complete event (with MSG_EOR set) was delivered.  A return value
>>> of -ENOMEM continues to indicate an out-of-memory condition was
>>> encountered.
>>>
>>> In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
>>> correct message reassembly logic for SCTP partial delivery.
>>> Change logic to ensure that as much data as possible is sent
>>> with the initial partial delivery and that following partial
>>> deliveries contain all available data.
>>>
>>> In sctp_ulpq_partial_delivery(), attempt partial delivery only
>>> if the data on the head of the reassembly queue is at or before
>>> the cumulative TSN ACK point.
>>>
>>> In sctp_ulpq_renege(), use the modified return values from
>>> sctp_ulpq_tail_data() to choose whether to attempt partial
>>> delivery or to attempt to drain the reassembly queue as a
>>> means to reduce memory pressure.  Remove call to
>>> sctp_tsnmap_mark(), as this is handled correctly in call to
>>> sctp_ulpq_tail_data().
>>>
>>> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
>>> ---
>>>    net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
>>>    1 file changed, 43 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
>>> index f221fbb..482e3ea 100644
>>> --- a/net/sctp/ulpqueue.c
>>> +++ b/net/sctp/ulpqueue.c
>>> @@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>>>    {
>>>    	struct sk_buff_head temp;
>>>    	struct sctp_ulpevent *event;
>>> +	int event_eor = 0;
>>>
>>>    	/* Create an event from the incoming chunk. */
>>>    	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
>>> @@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>>>    	/* Send event to the ULP.  'event' is the sctp_ulpevent for
>>>    	 * very first SKB on the 'temp' list.
>>>    	 */
>>> -	if (event)
>>> +	if (event) {
>>> +		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
>>>    		sctp_ulpq_tail_event(ulpq, event);
>>> +	}
>>
>> You can re-use the check just before this one to catch the EOR
>> condition.   You already do
>> 	if ((event) && (event->msg_flags & MSG_EOR)){
>>
>> right after you try to get a reassembled event.
>>
>> Everything else looks good.
>> -vlad
>>
> It depends on what we want the return value to mean.  In the case where we've reneged
> to be able to put in a packet, we know that the incoming packet is the next one we're
> looking for.  In which case, the earlier test is correct, since this will be the next
> event.
>
> In general, the event that's being passed to sctp_ulpq_order() might not be the
> next event.  In that case, sctp_ulpq_order() will call sctp_ulpq_store_ordered()
> and return NULL.  Only at the last "if (event)" can we really be sure that an event
> is being passed to the ULP.
>
> In general, do we want to return 1 if a complete event is actually passed to the ULP?
> Or, do we want to return 1 if we've seen a complete event come from reassembly?

You are right.  We want it set based on deliver to ULP.  There is a 
probability (if PARTIAL_DELIVERY_POINT is set) that we might get
a partial message here without EOR and skip ordering.  So, your check
is correct.

-vlad
>
>                                                      - Lee
>
>>>
>>> -	return 0;
>>> +	return event_eor;
>>>    }
>>>
>>>    /* Add a new event for propagation to the ULP.  */
>>> @@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq
>> *ulpq)
>>>    		ctsn = cevent->tsn;
>>>
>>>    		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
>>> +		case SCTP_DATA_FIRST_FRAG:
>>> +			if (!first_frag)
>>> +				return NULL;
>>> +			goto done;
>>>    		case SCTP_DATA_MIDDLE_FRAG:
>>>    			if (!first_frag) {
>>>    				first_frag = pos;
>>>    				next_tsn = ctsn + 1;
>>>    				last_frag = pos;
>>> -			} else if (next_tsn == ctsn)
>>> +			} else if (next_tsn == ctsn) {
>>>    				next_tsn++;
>>> -			else
>>> +				last_frag = pos;
>>> +			} else
>>>    				goto done;
>>>    			break;
>>>    		case SCTP_DATA_LAST_FRAG:
>>> @@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
>>>    			} else
>>>    				goto done;
>>>    			break;
>>> +
>>> +		case SCTP_DATA_LAST_FRAG:
>>> +			if (!first_frag)
>>> +				return NULL;
>>> +			else
>>> +				goto done;
>>> +			break;
>>> +
>>>    		default:
>>>    			return NULL;
>>>    		}
>>> @@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
>>>    	struct sctp_ulpevent *event;
>>>    	struct sctp_association *asoc;
>>>    	struct sctp_sock *sp;
>>> +	__u32 ctsn;
>>> +	struct sk_buff *skb;
>>>
>>>    	asoc = ulpq->asoc;
>>>    	sp = sctp_sk(asoc->base.sk);
>>>
>>>    	/* If the association is already in Partial Delivery mode
>>> -	 * we have noting to do.
>>> +	 * we have nothing to do.
>>>    	 */
>>>    	if (ulpq->pd_mode)
>>>    		return;
>>>
>>> +	/* Data must be at or below the Cumulative TSN ACK Point to
>>> +	 * start partial delivery.
>>> +	 */
>>> +	skb = skb_peek(&asoc->ulpq.reasm);
>>> +	if (skb != NULL) {
>>> +		ctsn = sctp_skb2event(skb)->tsn;
>>> +		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
>>> +			return;
>>> +	}
>>> +
>>>    	/* If the user enabled fragment interleave socket option,
>>>    	 * multiple associations can enter partial delivery.
>>>    	 * Otherwise, we can only enter partial delivery if the
>>> @@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>>>    	}
>>>    	/* If able to free enough room, accept this chunk. */
>>>    	if (chunk && (freed >= needed)) {
>>> -		__u32 tsn;
>>> -		tsn = ntohl(chunk->subh.data_hdr->tsn);
>>> -		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
>>> -		sctp_ulpq_tail_data(ulpq, chunk, gfp);
>>> -
>>> -		sctp_ulpq_partial_delivery(ulpq, gfp);
>>> +		int retval;
>>> +		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
>>> +		/*
>>> +		 * Enter partial delivery if chunk has not been
>>> +		 * delivered; otherwise, drain the reassembly queue.
>>> +		 */
>>> +		if (retval <= 0)
>>> +			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
>>> +		else if (retval == 1)
>>> +			sctp_ulpq_reasm_drain(ulpq);
>>>    	}
>>>
>>>    	sk_mem_reclaim(asoc->base.sk);
>>>
>

Re: [PATCH 4/4] sctp: fix association hangs due to partial delivery errors

[Vlad Yasevich]

On 02/26/2013 09:36 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
>
> In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
> a complete event (with MSG_EOR set) was delivered.  A return value
> of -ENOMEM continues to indicate an out-of-memory condition was
> encountered.
>
> In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
> correct message reassembly logic for SCTP partial delivery.
> Change logic to ensure that as much data as possible is sent
> with the initial partial delivery and that following partial
> deliveries contain all available data.
>
> In sctp_ulpq_partial_delivery(), attempt partial delivery only
> if the data on the head of the reassembly queue is at or before
> the cumulative TSN ACK point.
>
> In sctp_ulpq_renege(), use the modified return values from
> sctp_ulpq_tail_data() to choose whether to attempt partial
> delivery or to attempt to drain the reassembly queue as a
> means to reduce memory pressure.  Remove call to
> sctp_tsnmap_mark(), as this is handled correctly in call to
> sctp_ulpq_tail_data().
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index f221fbb..482e3ea 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   {
>   	struct sk_buff_head temp;
>   	struct sctp_ulpevent *event;
> +	int event_eor = 0;
>
>   	/* Create an event from the incoming chunk. */
>   	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
> @@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   	/* Send event to the ULP.  'event' is the sctp_ulpevent for
>   	 * very first SKB on the 'temp' list.
>   	 */
> -	if (event)
> +	if (event) {
> +		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
>   		sctp_ulpq_tail_event(ulpq, event);
> +	}
>
> -	return 0;
> +	return event_eor;
>   }
>
>   /* Add a new event for propagation to the ULP.  */
> @@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
>   		ctsn = cevent->tsn;
>
>   		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
> +		case SCTP_DATA_FIRST_FRAG:
> +			if (!first_frag)
> +				return NULL;
> +			goto done;
>   		case SCTP_DATA_MIDDLE_FRAG:
>   			if (!first_frag) {
>   				first_frag = pos;
>   				next_tsn = ctsn + 1;
>   				last_frag = pos;
> -			} else if (next_tsn == ctsn)
> +			} else if (next_tsn == ctsn) {
>   				next_tsn++;
> -			else
> +				last_frag = pos;
> +			} else
>   				goto done;
>   			break;
>   		case SCTP_DATA_LAST_FRAG:
> @@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
>   			} else
>   				goto done;
>   			break;
> +
> +		case SCTP_DATA_LAST_FRAG:
> +			if (!first_frag)
> +				return NULL;
> +			else
> +				goto done;
> +			break;
> +
>   		default:
>   			return NULL;
>   		}
> @@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
>   	struct sctp_ulpevent *event;
>   	struct sctp_association *asoc;
>   	struct sctp_sock *sp;
> +	__u32 ctsn;
> +	struct sk_buff *skb;
>
>   	asoc = ulpq->asoc;
>   	sp = sctp_sk(asoc->base.sk);
>
>   	/* If the association is already in Partial Delivery mode
> -	 * we have noting to do.
> +	 * we have nothing to do.
>   	 */
>   	if (ulpq->pd_mode)
>   		return;
>
> +	/* Data must be at or below the Cumulative TSN ACK Point to
> +	 * start partial delivery.
> +	 */
> +	skb = skb_peek(&asoc->ulpq.reasm);
> +	if (skb != NULL) {
> +		ctsn = sctp_skb2event(skb)->tsn;
> +		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
> +			return;
> +	}
> +
>   	/* If the user enabled fragment interleave socket option,
>   	 * multiple associations can enter partial delivery.
>   	 * Otherwise, we can only enter partial delivery if the
> @@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>   	}
>   	/* If able to free enough room, accept this chunk. */
>   	if (chunk && (freed >= needed)) {
> -		__u32 tsn;
> -		tsn = ntohl(chunk->subh.data_hdr->tsn);
> -		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
> -		sctp_ulpq_tail_data(ulpq, chunk, gfp);
> -
> -		sctp_ulpq_partial_delivery(ulpq, gfp);
> +		int retval;
> +		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
> +		/*
> +		 * Enter partial delivery if chunk has not been
> +		 * delivered; otherwise, drain the reassembly queue.
> +		 */
> +		if (retval <= 0)
> +			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
> +		else if (retval == 1)
> +			sctp_ulpq_reasm_drain(ulpq);
>   	}
>
>   	sk_mem_reclaim(asoc->base.sk);
>

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: "Lee A. Roberts" <lee.roberts@hp.com>
Date: Tue, 26 Feb 2013 07:36:12 -0700

> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.
> 
> Lee A. Roberts (4):
>   sctp: fix association hangs due to off-by-one errors in
>     sctp_tsnmap_grow()
>   sctp: fix association hangs due to reneging packets below the
>     cumulative TSN ACK point
>   sctp: fix association hangs due to errors when reneging events from
>     the ordering queue
>   sctp: fix association hangs due to partial delivery errors

Vlad, Sridhar, and Neil, please review.

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Neil Horman]

On Tue, Feb 26, 2013 at 05:37:58PM -0500, David Miller wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> Date: Tue, 26 Feb 2013 07:36:12 -0700
> 
> > This series of patches resolves several SCTP association hangs observed during
> > SCTP stress testing.  Observable symptoms include communications hangs with
> > data being held in the association reassembly and/or lobby (ordering) queues.
> > Close examination of reassembly/ordering queues may show either duplicated
> > or missing packets.
> > 
> > Lee A. Roberts (4):
> >   sctp: fix association hangs due to off-by-one errors in
> >     sctp_tsnmap_grow()
> >   sctp: fix association hangs due to reneging packets below the
> >     cumulative TSN ACK point
> >   sctp: fix association hangs due to errors when reneging events from
> >     the ordering queue
> >   sctp: fix association hangs due to partial delivery errors
> 
> Vlad, Sridhar, and Neil, please review.
> 
IIRC, Vlad already/still has some outstanding comments & questions regarding
this series.
Neil

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Sridhar Samudrala]

On 2/26/2013 2:37 PM, David Miller wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> Date: Tue, 26 Feb 2013 07:36:12 -0700
>
>> This series of patches resolves several SCTP association hangs observed during
>> SCTP stress testing.  Observable symptoms include communications hangs with
>> data being held in the association reassembly and/or lobby (ordering) queues.
>> Close examination of reassembly/ordering queues may show either duplicated
>> or missing packets.
>>
>> Lee A. Roberts (4):
>>    sctp: fix association hangs due to off-by-one errors in
>>      sctp_tsnmap_grow()
>>    sctp: fix association hangs due to reneging packets below the
>>      cumulative TSN ACK point
>>    sctp: fix association hangs due to errors when reneging events from
>>      the ordering queue
>>    sctp: fix association hangs due to partial delivery errors
> Vlad, Sridhar, and Neil, please review.

I am no longer able to spend time on reviewing SCTP patches.
Please remove my name from the maintainers list. Should i submit a patch ?

Thanks
Sridhar

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Neil Horman]

On Wed, Feb 27, 2013 at 08:41:33AM -0800, Sridhar Samudrala wrote:
> On 2/26/2013 2:37 PM, David Miller wrote:
> >From: "Lee A. Roberts" <lee.roberts@hp.com>
> >Date: Tue, 26 Feb 2013 07:36:12 -0700
> >
> >>This series of patches resolves several SCTP association hangs observed during
> >>SCTP stress testing.  Observable symptoms include communications hangs with
> >>data being held in the association reassembly and/or lobby (ordering) queues.
> >>Close examination of reassembly/ordering queues may show either duplicated
> >>or missing packets.
> >>
> >>Lee A. Roberts (4):
> >>   sctp: fix association hangs due to off-by-one errors in
> >>     sctp_tsnmap_grow()
> >>   sctp: fix association hangs due to reneging packets below the
> >>     cumulative TSN ACK point
> >>   sctp: fix association hangs due to errors when reneging events from
> >>     the ordering queue
> >>   sctp: fix association hangs due to partial delivery errors
> >Vlad, Sridhar, and Neil, please review.
> 
> I am no longer able to spend time on reviewing SCTP patches.
> Please remove my name from the maintainers list. Should i submit a patch ?
> 
> Thanks
> Sridhar
> 
> 
Yes, please. 

Thanks!
Neil

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: "Lee A. Roberts" <lee.roberts@hp.com>
Date: Tue, 26 Feb 2013 07:36:12 -0700

> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.
> 
> Lee A. Roberts (4):
>   sctp: fix association hangs due to off-by-one errors in
>     sctp_tsnmap_grow()
>   sctp: fix association hangs due to reneging packets below the
>     cumulative TSN ACK point
>   sctp: fix association hangs due to errors when reneging events from
>     the ordering queue
>   sctp: fix association hangs due to partial delivery errors

Appying these patches breaks the build:

net/sctp/ulpqueue.c: In function ‘sctp_ulpq_renege’:
net/sctp/ulpqueue.c:1115:4: warning: passing argument 2 of ‘sctp_ulpq_partial_delivery’ makes integer from pointer without a cast [enabled by default]
net/sctp/ulpqueue.c:1038:6: note: expected ‘gfp_t’ but argument is of type ‘struct sctp_chunk *’
net/sctp/ulpqueue.c:1115:4: error: too many arguments to function ‘sctp_ulpq_partial_delivery’
net/sctp/ulpqueue.c:1038:6: note: declared here

RE: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Roberts, Lee A.]

I missed the change in the calling sequence for sctp_ulpq_partial_delivery()
when porting the changes forward from 3.2.x.  An updated patch has been sent.

                                             - Lee Roberts

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Wednesday, February 27, 2013 11:10 AM
To: Roberts, Lee A.
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic
Importance: High

From: "Lee A. Roberts" <lee.roberts@hp.com>
Date: Tue, 26 Feb 2013 07:36:12 -0700

> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.
> 
> Lee A. Roberts (4):
>   sctp: fix association hangs due to off-by-one errors in
>     sctp_tsnmap_grow()
>   sctp: fix association hangs due to reneging packets below the
>     cumulative TSN ACK point
>   sctp: fix association hangs due to errors when reneging events from
>     the ordering queue
>   sctp: fix association hangs due to partial delivery errors

Appying these patches breaks the build:

net/sctp/ulpqueue.c: In function ‘sctp_ulpq_renege’:
net/sctp/ulpqueue.c:1115:4: warning: passing argument 2 of ‘sctp_ulpq_partial_delivery’ makes integer from pointer without a cast [enabled by default]
net/sctp/ulpqueue.c:1038:6: note: expected ‘gfp_t’ but argument is of type ‘struct sctp_chunk *’
net/sctp/ulpqueue.c:1115:4: error: too many arguments to function ‘sctp_ulpq_partial_delivery’
net/sctp/ulpqueue.c:1038:6: note: declared here

Re: [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: "Roberts, Lee A." <lee.roberts@hp.com>
Date: Wed, 27 Feb 2013 18:55:36 +0000

> I missed the change in the calling sequence for sctp_ulpq_partial_delivery()
> when porting the changes forward from 3.2.x.  An updated patch has been sent.

Thanks.  But please CC: the SCTP maintainers or at least CC: the
linux-sctp list, as well as netdev, when you post SCTP patches,
thanks.

[PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

This series of patches resolves several SCTP association hangs observed during
SCTP stress testing.  Observable symptoms include communications hangs with
data being held in the association reassembly and/or lobby (ordering) queues.
Close examination of reassembly/ordering queues may show either duplicated
or missing packets.

Lee A. Roberts (4):
  sctp: fix association hangs due to off-by-one errors in
    sctp_tsnmap_grow()
  sctp: fix association hangs due to reneging packets below the
    cumulative TSN ACK point
  sctp: fix association hangs due to errors when reneging events from
    the ordering queue
  sctp: fix association hangs due to partial delivery errors

 net/sctp/tsnmap.c   |   13 ++++----
 net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 78 insertions(+), 22 deletions(-)

-- 
1.7.9.5

Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Daniel Borkmann]

On 02/27/2013 07:54 PM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.

Just nitpicking here ... all of your 4 patches have the same first paragraph,
which can actually be left out completely if it is already in your patch cover
letter stated.

> Lee A. Roberts (4):
>    sctp: fix association hangs due to off-by-one errors in
>      sctp_tsnmap_grow()
>    sctp: fix association hangs due to reneging packets below the
>      cumulative TSN ACK point
>    sctp: fix association hangs due to errors when reneging events from
>      the ordering queue
>    sctp: fix association hangs due to partial delivery errors
>
>   net/sctp/tsnmap.c   |   13 ++++----
>   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 78 insertions(+), 22 deletions(-)

Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 27 Feb 2013 20:36:50 +0100

> On 02/27/2013 07:54 PM, Lee A. Roberts wrote:
>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>
>> This series of patches resolves several SCTP association hangs
>> observed during
>> SCTP stress testing.  Observable symptoms include communications hangs
>> with
>> data being held in the association reassembly and/or lobby (ordering)
>> queues.
>> Close examination of reassembly/ordering queues may show either
>> duplicated
>> or missing packets.
> 
> Just nitpicking here ... all of your 4 patches have the same first
> paragraph,
> which can actually be left out completely if it is already in your
> patch cover
> letter stated.

Right because I can create a merge commit for the series when I apply
it and put that text there.

Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Vlad Yasevich]

On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.

Hi Lee

What changed in this series?  I looked through and they don't look 
different from the prior version?

-vlad

>
> Lee A. Roberts (4):
>    sctp: fix association hangs due to off-by-one errors in
>      sctp_tsnmap_grow()
>    sctp: fix association hangs due to reneging packets below the
>      cumulative TSN ACK point
>    sctp: fix association hangs due to errors when reneging events from
>      the ordering queue
>    sctp: fix association hangs due to partial delivery errors
>
>   net/sctp/tsnmap.c   |   13 ++++----
>   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 78 insertions(+), 22 deletions(-)
>

Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 27 Feb 2013 15:13:57 -0500

> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>
>> This series of patches resolves several SCTP association hangs
>> observed during
>> SCTP stress testing.  Observable symptoms include communications hangs
>> with
>> data being held in the association reassembly and/or lobby (ordering)
>> queues.
>> Close examination of reassembly/ordering queues may show either
>> duplicated
>> or missing packets.
> 
> Hi Lee
> 
> What changed in this series?  I looked through and they don't look
> different from the prior version?

He fixed the build failure, and he described this at:

http://marc.info/?l=linux-netdev&m=136199143224349&w=2

But yeah he absolutely and positively should have mentioned it here
too.

Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Vlad Yasevich]

On 02/27/2013 03:24 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Wed, 27 Feb 2013 15:13:57 -0500
>
>> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
>>> From: "Lee A. Roberts" <lee.roberts@hp.com>
>>>
>>> This series of patches resolves several SCTP association hangs
>>> observed during
>>> SCTP stress testing.  Observable symptoms include communications hangs
>>> with
>>> data being held in the association reassembly and/or lobby (ordering)
>>> queues.
>>> Close examination of reassembly/ordering queues may show either
>>> duplicated
>>> or missing packets.
>>
>> Hi Lee
>>
>> What changed in this series?  I looked through and they don't look
>> different from the prior version?
>
> He fixed the build failure, and he described this at:
>
> http://marc.info/?l=linux-netdev&m=136199143224349&w=2
>
> But yeah he absolutely and positively should have mentioned it here
> too.
>

Thanks.  Didn't realize it was just a build error.  Was looking for 
functionality change.

-vlad

RE: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Roberts, Lee A.]

> -----Original Message-----
> From: Vlad Yasevich [mailto:vyasevic@redhat.com]
> Sent: Wednesday, February 27, 2013 1:14 PM
> To: Roberts, Lee A.
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH v2 0/4] sctp: fix association hangs due to reassembly/ordering logic
> 
> On 02/27/2013 01:54 PM, Lee A. Roberts wrote:
> > From: "Lee A. Roberts" <lee.roberts@hp.com>
> >
> > This series of patches resolves several SCTP association hangs observed during
> > SCTP stress testing.  Observable symptoms include communications hangs with
> > data being held in the association reassembly and/or lobby (ordering) queues.
> > Close examination of reassembly/ordering queues may show either duplicated
> > or missing packets.
> 
> Hi Lee
> 
> What changed in this series?  I looked through and they don't look
> different from the prior version?
> 
> -vlad
> 

Vlad,

In patch #4, routine sctp_ulpq_renege(), the inserted call to sctp_ulpq_partial_delivery()
still used the 3.2.x calling sequence with three arguments.

The first version of patch #4 stated:

+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);

The second version of patch #4 has corrected the call to sctp_ulpq_partial_delivery():

+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);

I have adjusted the patch comments for yet another version of the patch series.
I'm holding them at the moment to collect any further change requests.

                                        - Lee



> >
> > Lee A. Roberts (4):
> >    sctp: fix association hangs due to off-by-one errors in
> >      sctp_tsnmap_grow()
> >    sctp: fix association hangs due to reneging packets below the
> >      cumulative TSN ACK point
> >    sctp: fix association hangs due to errors when reneging events from
> >      the ordering queue
> >    sctp: fix association hangs due to partial delivery errors
> >
> >   net/sctp/tsnmap.c   |   13 ++++----
> >   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
> >   2 files changed, 78 insertions(+), 22 deletions(-)
> >

[PATCH v3 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

This series of patches resolves several SCTP association hangs observed during
SCTP stress testing.  Observable symptoms include communications hangs with
data being held in the association reassembly and/or lobby (ordering) queues.
Close examination of reassembly/ordering queues may show either duplicated
or missing packets.

In version #2, corrected build failure in initial version of patch series
due to wrong calling sequence for sctp_ulpq_partial_delivery() being inserted
in sctp_ulpq_renege().

In version #3, adjusted patch documentation to be less repetitive.

Lee A. Roberts (4):
  sctp: fix association hangs due to off-by-one errors in
    sctp_tsnmap_grow()
  sctp: fix association hangs due to reneging packets below the
    cumulative TSN ACK point
  sctp: fix association hangs due to errors when reneging events from
    the ordering queue
  sctp: fix association hangs due to partial delivery errors

 net/sctp/tsnmap.c   |   13 ++++----
 net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 78 insertions(+), 22 deletions(-)

-- 
1.7.9.5

Re: [PATCH v3 0/4] sctp: fix association hangs due to reassembly/ordering logic

[Vlad Yasevich]

On 02/28/2013 09:37 AM, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
>
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.
>
> In version #2, corrected build failure in initial version of patch series
> due to wrong calling sequence for sctp_ulpq_partial_delivery() being inserted
> in sctp_ulpq_renege().
>
> In version #3, adjusted patch documentation to be less repetitive.

My Acks from version 1 still apply.

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

>
> Lee A. Roberts (4):
>    sctp: fix association hangs due to off-by-one errors in
>      sctp_tsnmap_grow()
>    sctp: fix association hangs due to reneging packets below the
>      cumulative TSN ACK point
>    sctp: fix association hangs due to errors when reneging events from
>      the ordering queue
>    sctp: fix association hangs due to partial delivery errors
>
>   net/sctp/tsnmap.c   |   13 ++++----
>   net/sctp/ulpqueue.c |   87 +++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 78 insertions(+), 22 deletions(-)
>

Re: [PATCH v3 0/4] sctp: fix association hangs due to reassembly/ordering logic

[David Miller]

From: "Lee A. Roberts" <lee.roberts@hp.com>
Date: Thu, 28 Feb 2013 07:37:26 -0700

> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> This series of patches resolves several SCTP association hangs observed during
> SCTP stress testing.  Observable symptoms include communications hangs with
> data being held in the association reassembly and/or lobby (ordering) queues.
> Close examination of reassembly/ordering queues may show either duplicated
> or missing packets.
> 
> In version #2, corrected build failure in initial version of patch series
> due to wrong calling sequence for sctp_ulpq_partial_delivery() being inserted
> in sctp_ulpq_renege().
> 
> In version #3, adjusted patch documentation to be less repetitive.

Series applied, thanks.

[PATCH v3 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

In sctp_tsnmap_mark(), correct off-by-one error when calculating
size value for sctp_tsnmap_grow().

In sctp_tsnmap_grow(), correct off-by-one error when copying
and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
word, this bit can be lost, causing the corresponding packet
to be transmitted again and to be entered as a duplicate into
the SCTP reassembly/ordering queues.  Change parameter name
from "gap" (zero-based index) to "size" (one-based) to enhance
code readability.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/tsnmap.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sctp/tsnmap.c b/net/sctp/tsnmap.c
index 5f25e0c..396c451 100644
--- a/net/sctp/tsnmap.c
+++ b/net/sctp/tsnmap.c
@@ -51,7 +51,7 @@
 static void sctp_tsnmap_update(struct sctp_tsnmap *map);
 static void sctp_tsnmap_find_gap_ack(unsigned long *map, __u16 off,
 				     __u16 len, __u16 *start, __u16 *end);
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap);
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size);
 
 /* Initialize a block of memory as a tsnmap.  */
 struct sctp_tsnmap *sctp_tsnmap_init(struct sctp_tsnmap *map, __u16 len,
@@ -124,7 +124,7 @@ int sctp_tsnmap_mark(struct sctp_tsnmap *map, __u32 tsn,
 
 	gap = tsn - map->base_tsn;
 
-	if (gap >= map->len && !sctp_tsnmap_grow(map, gap))
+	if (gap >= map->len && !sctp_tsnmap_grow(map, gap + 1))
 		return -ENOMEM;
 
 	if (!sctp_tsnmap_has_gap(map) && gap == 0) {
@@ -360,23 +360,24 @@ __u16 sctp_tsnmap_num_gabs(struct sctp_tsnmap *map,
 	return ngaps;
 }
 
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap)
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size)
 {
 	unsigned long *new;
 	unsigned long inc;
 	u16  len;
 
-	if (gap >= SCTP_TSN_MAP_SIZE)
+	if (size > SCTP_TSN_MAP_SIZE)
 		return 0;
 
-	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
+	inc = ALIGN((size - map->len), BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
 	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
 
 	new = kzalloc(len>>3, GFP_ATOMIC);
 	if (!new)
 		return 0;
 
-	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
+	bitmap_copy(new, map->tsn_map,
+		map->max_tsn_seen - map->cumulative_tsn_ack_point);
 	kfree(map->tsn_map);
 	map->tsn_map = new;
 	map->len = len;
-- 
1.7.9.5

[PATCH v3 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

In sctp_ulpq_renege_list(), do not renege packets below the
cumulative TSN ACK point.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index ada1746..63afddc 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -969,11 +969,16 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 
 	tsnmap = &ulpq->asoc->peer.tsn_map;
 
-	while ((skb = __skb_dequeue_tail(list)) != NULL) {
-		freed += skb_headlen(skb);
+	while ((skb = skb_peek_tail(list)) != NULL) {
 		event = sctp_skb2event(skb);
 		tsn = event->tsn;
 
+		/* Don't renege below the Cumulative TSN ACK Point. */
+		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
+			break;
+
+		__skb_unlink(skb, list);
+		freed += skb_headlen(skb);
 		sctp_ulpevent_free(event);
 		sctp_tsnmap_renege(tsnmap, tsn);
 		if (freed >= needed)
-- 
1.7.9.5

[PATCH v3 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

In sctp_ulpq_renege_list(), events being reneged from the
ordering queue may correspond to multiple TSNs.  Identify
all affected packets; sum freed space and renege from the
tsnmap.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 63afddc..f221fbb 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -962,8 +962,8 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		struct sk_buff_head *list, __u16 needed)
 {
 	__u16 freed = 0;
-	__u32 tsn;
-	struct sk_buff *skb;
+	__u32 tsn, last_tsn;
+	struct sk_buff *skb, *flist, *last;
 	struct sctp_ulpevent *event;
 	struct sctp_tsnmap *tsnmap;
 
@@ -977,10 +977,28 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
 			break;
 
-		__skb_unlink(skb, list);
+		/* Events in ordering queue may have multiple fragments
+		 * corresponding to additional TSNs.  Sum the total
+		 * freed space; find the last TSN.
+		 */
 		freed += skb_headlen(skb);
+		flist = skb_shinfo(skb)->frag_list;
+		for (last = flist; flist; flist = flist->next) {
+			last = flist;
+			freed += skb_headlen(last);
+		}
+		if (last)
+			last_tsn = sctp_skb2event(last)->tsn;
+		else
+			last_tsn = tsn;
+
+		/* Unlink the event, then renege all applicable TSNs. */
+		__skb_unlink(skb, list);
 		sctp_ulpevent_free(event);
-		sctp_tsnmap_renege(tsnmap, tsn);
+		while (TSN_lte(tsn, last_tsn)) {
+			sctp_tsnmap_renege(tsnmap, tsn);
+			tsn++;
+		}
 		if (freed >= needed)
 			return freed;
 	}
-- 
1.7.9.5

[PATCH v3 4/4] sctp: fix association hangs due to partial delivery errors

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
a complete event (with MSG_EOR set) was delivered.  A return value
of -ENOMEM continues to indicate an out-of-memory condition was
encountered.

In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
correct message reassembly logic for SCTP partial delivery.
Change logic to ensure that as much data as possible is sent
with the initial partial delivery and that following partial
deliveries contain all available data.

In sctp_ulpq_partial_delivery(), attempt partial delivery only
if the data on the head of the reassembly queue is at or before
the cumulative TSN ACK point.

In sctp_ulpq_renege(), use the modified return values from
sctp_ulpq_tail_data() to choose whether to attempt partial
delivery or to attempt to drain the reassembly queue as a
means to reduce memory pressure.  Remove call to
sctp_tsnmap_mark(), as this is handled correctly in call to
sctp_ulpq_tail_data().

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index f221fbb..482e3ea 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 {
 	struct sk_buff_head temp;
 	struct sctp_ulpevent *event;
+	int event_eor = 0;
 
 	/* Create an event from the incoming chunk. */
 	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
@@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	/* Send event to the ULP.  'event' is the sctp_ulpevent for
 	 * very first SKB on the 'temp' list.
 	 */
-	if (event)
+	if (event) {
+		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
 		sctp_ulpq_tail_event(ulpq, event);
+	}
 
-	return 0;
+	return event_eor;
 }
 
 /* Add a new event for propagation to the ULP.  */
@@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
 		ctsn = cevent->tsn;
 
 		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
+		case SCTP_DATA_FIRST_FRAG:
+			if (!first_frag)
+				return NULL;
+			goto done;
 		case SCTP_DATA_MIDDLE_FRAG:
 			if (!first_frag) {
 				first_frag = pos;
 				next_tsn = ctsn + 1;
 				last_frag = pos;
-			} else if (next_tsn == ctsn)
+			} else if (next_tsn == ctsn) {
 				next_tsn++;
-			else
+				last_frag = pos;
+			} else
 				goto done;
 			break;
 		case SCTP_DATA_LAST_FRAG:
@@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
 			} else
 				goto done;
 			break;
+
+		case SCTP_DATA_LAST_FRAG:
+			if (!first_frag)
+				return NULL;
+			else
+				goto done;
+			break;
+
 		default:
 			return NULL;
 		}
@@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
 	struct sctp_ulpevent *event;
 	struct sctp_association *asoc;
 	struct sctp_sock *sp;
+	__u32 ctsn;
+	struct sk_buff *skb;
 
 	asoc = ulpq->asoc;
 	sp = sctp_sk(asoc->base.sk);
 
 	/* If the association is already in Partial Delivery mode
-	 * we have noting to do.
+	 * we have nothing to do.
 	 */
 	if (ulpq->pd_mode)
 		return;
 
+	/* Data must be at or below the Cumulative TSN ACK Point to
+	 * start partial delivery.
+	 */
+	skb = skb_peek(&asoc->ulpq.reasm);
+	if (skb != NULL) {
+		ctsn = sctp_skb2event(skb)->tsn;
+		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
+			return;
+	}
+
 	/* If the user enabled fragment interleave socket option,
 	 * multiple associations can enter partial delivery.
 	 * Otherwise, we can only enter partial delivery if the
@@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	}
 	/* If able to free enough room, accept this chunk. */
 	if (chunk && (freed >= needed)) {
-		__u32 tsn;
-		tsn = ntohl(chunk->subh.data_hdr->tsn);
-		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
-		sctp_ulpq_tail_data(ulpq, chunk, gfp);
-
-		sctp_ulpq_partial_delivery(ulpq, gfp);
+		int retval;
+		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
+		/*
+		 * Enter partial delivery if chunk has not been
+		 * delivered; otherwise, drain the reassembly queue.
+		 */
+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);
 	}
 
 	sk_mem_reclaim(asoc->base.sk);
-- 
1.7.9.5

[PATCH v2 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association lobby (ordering)
queue.  Close examination of reassembly/ordering queues shows
duplicated packets.

In sctp_tsnmap_mark(), correct off-by-one error when calculating
size value for sctp_tsnmap_grow().

In sctp_tsnmap_grow(), correct off-by-one error when copying
and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
word, this bit can be lost, causing the corresponding packet
to be transmitted again and to be entered as a duplicate into
the SCTP reassembly/ordering queues.  Change parameter name
from "gap" (zero-based index) to "size" (one-based) to enhance
code readability.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/tsnmap.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sctp/tsnmap.c b/net/sctp/tsnmap.c
index 5f25e0c..396c451 100644
--- a/net/sctp/tsnmap.c
+++ b/net/sctp/tsnmap.c
@@ -51,7 +51,7 @@
 static void sctp_tsnmap_update(struct sctp_tsnmap *map);
 static void sctp_tsnmap_find_gap_ack(unsigned long *map, __u16 off,
 				     __u16 len, __u16 *start, __u16 *end);
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap);
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size);
 
 /* Initialize a block of memory as a tsnmap.  */
 struct sctp_tsnmap *sctp_tsnmap_init(struct sctp_tsnmap *map, __u16 len,
@@ -124,7 +124,7 @@ int sctp_tsnmap_mark(struct sctp_tsnmap *map, __u32 tsn,
 
 	gap = tsn - map->base_tsn;
 
-	if (gap >= map->len && !sctp_tsnmap_grow(map, gap))
+	if (gap >= map->len && !sctp_tsnmap_grow(map, gap + 1))
 		return -ENOMEM;
 
 	if (!sctp_tsnmap_has_gap(map) && gap == 0) {
@@ -360,23 +360,24 @@ __u16 sctp_tsnmap_num_gabs(struct sctp_tsnmap *map,
 	return ngaps;
 }
 
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap)
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size)
 {
 	unsigned long *new;
 	unsigned long inc;
 	u16  len;
 
-	if (gap >= SCTP_TSN_MAP_SIZE)
+	if (size > SCTP_TSN_MAP_SIZE)
 		return 0;
 
-	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
+	inc = ALIGN((size - map->len), BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
 	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
 
 	new = kzalloc(len>>3, GFP_ATOMIC);
 	if (!new)
 		return 0;
 
-	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
+	bitmap_copy(new, map->tsn_map,
+		map->max_tsn_seen - map->cumulative_tsn_ack_point);
 	kfree(map->tsn_map);
 	map->tsn_map = new;
 	map->len = len;
-- 
1.7.9.5

Re: [PATCH v2 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Neil Horman]

On Wed, Feb 27, 2013 at 11:54:30AM -0700, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association lobby (ordering)
> queue.  Close examination of reassembly/ordering queues shows
> duplicated packets.
> 
> In sctp_tsnmap_mark(), correct off-by-one error when calculating
> size value for sctp_tsnmap_grow().
> 
> In sctp_tsnmap_grow(), correct off-by-one error when copying
> and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
> word, this bit can be lost, causing the corresponding packet
> to be transmitted again and to be entered as a duplicate into
> the SCTP reassembly/ordering queues.  Change parameter name
> from "gap" (zero-based index) to "size" (one-based) to enhance
> code readability.
> 
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

[PATCH v2 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_renege_list(), do not renege packets below the
cumulative TSN ACK point.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index ada1746..63afddc 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -969,11 +969,16 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 
 	tsnmap = &ulpq->asoc->peer.tsn_map;
 
-	while ((skb = __skb_dequeue_tail(list)) != NULL) {
-		freed += skb_headlen(skb);
+	while ((skb = skb_peek_tail(list)) != NULL) {
 		event = sctp_skb2event(skb);
 		tsn = event->tsn;
 
+		/* Don't renege below the Cumulative TSN ACK Point. */
+		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
+			break;
+
+		__skb_unlink(skb, list);
+		freed += skb_headlen(skb);
 		sctp_ulpevent_free(event);
 		sctp_tsnmap_renege(tsnmap, tsn);
 		if (freed >= needed)
-- 
1.7.9.5

Re: [PATCH v2 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point

[Neil Horman]

On Wed, Feb 27, 2013 at 11:54:31AM -0700, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
> 
> In sctp_ulpq_renege_list(), do not renege packets below the
> cumulative TSN ACK point.
> 
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> 

[PATCH v2 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_renege_list(), events being reneged from the
ordering queue may correspond to multiple TSNs.  Identify
all affected packets; sum freed space and renege from the
tsnmap.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 63afddc..f221fbb 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -962,8 +962,8 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		struct sk_buff_head *list, __u16 needed)
 {
 	__u16 freed = 0;
-	__u32 tsn;
-	struct sk_buff *skb;
+	__u32 tsn, last_tsn;
+	struct sk_buff *skb, *flist, *last;
 	struct sctp_ulpevent *event;
 	struct sctp_tsnmap *tsnmap;
 
@@ -977,10 +977,28 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
 		if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
 			break;
 
-		__skb_unlink(skb, list);
+		/* Events in ordering queue may have multiple fragments
+		 * corresponding to additional TSNs.  Sum the total
+		 * freed space; find the last TSN.
+		 */
 		freed += skb_headlen(skb);
+		flist = skb_shinfo(skb)->frag_list;
+		for (last = flist; flist; flist = flist->next) {
+			last = flist;
+			freed += skb_headlen(last);
+		}
+		if (last)
+			last_tsn = sctp_skb2event(last)->tsn;
+		else
+			last_tsn = tsn;
+
+		/* Unlink the event, then renege all applicable TSNs. */
+		__skb_unlink(skb, list);
 		sctp_ulpevent_free(event);
-		sctp_tsnmap_renege(tsnmap, tsn);
+		while (TSN_lte(tsn, last_tsn)) {
+			sctp_tsnmap_renege(tsnmap, tsn);
+			tsn++;
+		}
 		if (freed >= needed)
 			return freed;
 	}
-- 
1.7.9.5

Re: [PATCH v2 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue

[Neil Horman]

On Wed, Feb 27, 2013 at 11:54:32AM -0700, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
> 
> In sctp_ulpq_renege_list(), events being reneged from the
> ordering queue may correspond to multiple TSNs.  Identify
> all affected packets; sum freed space and renege from the
> tsnmap.
> 
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 

[PATCH v2 4/4] sctp: fix association hangs due to partial delivery errors

[Lee A. Roberts]

From: "Lee A. Roberts" <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues.  Close examination of reassembly queue shows
missing packets.

In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
a complete event (with MSG_EOR set) was delivered.  A return value
of -ENOMEM continues to indicate an out-of-memory condition was
encountered.

In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
correct message reassembly logic for SCTP partial delivery.
Change logic to ensure that as much data as possible is sent
with the initial partial delivery and that following partial
deliveries contain all available data.

In sctp_ulpq_partial_delivery(), attempt partial delivery only
if the data on the head of the reassembly queue is at or before
the cumulative TSN ACK point.

In sctp_ulpq_renege(), use the modified return values from
sctp_ulpq_tail_data() to choose whether to attempt partial
delivery or to attempt to drain the reassembly queue as a
means to reduce memory pressure.  Remove call to
sctp_tsnmap_mark(), as this is handled correctly in call to
sctp_ulpq_tail_data().

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/ulpqueue.c |   54 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index f221fbb..482e3ea 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 {
 	struct sk_buff_head temp;
 	struct sctp_ulpevent *event;
+	int event_eor = 0;
 
 	/* Create an event from the incoming chunk. */
 	event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
@@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	/* Send event to the ULP.  'event' is the sctp_ulpevent for
 	 * very first SKB on the 'temp' list.
 	 */
-	if (event)
+	if (event) {
+		event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
 		sctp_ulpq_tail_event(ulpq, event);
+	}
 
-	return 0;
+	return event_eor;
 }
 
 /* Add a new event for propagation to the ULP.  */
@@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
 		ctsn = cevent->tsn;
 
 		switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
+		case SCTP_DATA_FIRST_FRAG:
+			if (!first_frag)
+				return NULL;
+			goto done;
 		case SCTP_DATA_MIDDLE_FRAG:
 			if (!first_frag) {
 				first_frag = pos;
 				next_tsn = ctsn + 1;
 				last_frag = pos;
-			} else if (next_tsn == ctsn)
+			} else if (next_tsn == ctsn) {
 				next_tsn++;
-			else
+				last_frag = pos;
+			} else
 				goto done;
 			break;
 		case SCTP_DATA_LAST_FRAG:
@@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
 			} else
 				goto done;
 			break;
+
+		case SCTP_DATA_LAST_FRAG:
+			if (!first_frag)
+				return NULL;
+			else
+				goto done;
+			break;
+
 		default:
 			return NULL;
 		}
@@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
 	struct sctp_ulpevent *event;
 	struct sctp_association *asoc;
 	struct sctp_sock *sp;
+	__u32 ctsn;
+	struct sk_buff *skb;
 
 	asoc = ulpq->asoc;
 	sp = sctp_sk(asoc->base.sk);
 
 	/* If the association is already in Partial Delivery mode
-	 * we have noting to do.
+	 * we have nothing to do.
 	 */
 	if (ulpq->pd_mode)
 		return;
 
+	/* Data must be at or below the Cumulative TSN ACK Point to
+	 * start partial delivery.
+	 */
+	skb = skb_peek(&asoc->ulpq.reasm);
+	if (skb != NULL) {
+		ctsn = sctp_skb2event(skb)->tsn;
+		if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
+			return;
+	}
+
 	/* If the user enabled fragment interleave socket option,
 	 * multiple associations can enter partial delivery.
 	 * Otherwise, we can only enter partial delivery if the
@@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 	}
 	/* If able to free enough room, accept this chunk. */
 	if (chunk && (freed >= needed)) {
-		__u32 tsn;
-		tsn = ntohl(chunk->subh.data_hdr->tsn);
-		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
-		sctp_ulpq_tail_data(ulpq, chunk, gfp);
-
-		sctp_ulpq_partial_delivery(ulpq, gfp);
+		int retval;
+		retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
+		/*
+		 * Enter partial delivery if chunk has not been
+		 * delivered; otherwise, drain the reassembly queue.
+		 */
+		if (retval <= 0)
+			sctp_ulpq_partial_delivery(ulpq, gfp);
+		else if (retval == 1)
+			sctp_ulpq_reasm_drain(ulpq);
 	}
 
 	sk_mem_reclaim(asoc->base.sk);
-- 
1.7.9.5

Re: [PATCH v2 4/4] sctp: fix association hangs due to partial delivery errors

[Neil Horman]

On Wed, Feb 27, 2013 at 11:54:33AM -0700, Lee A. Roberts wrote:
> From: "Lee A. Roberts" <lee.roberts@hp.com>
> 
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association reassembly and/or lobby
> (ordering) queues.  Close examination of reassembly queue shows
> missing packets.
> 
> In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
> a complete event (with MSG_EOR set) was delivered.  A return value
> of -ENOMEM continues to indicate an out-of-memory condition was
> encountered.
> 
> In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
> correct message reassembly logic for SCTP partial delivery.
> Change logic to ensure that as much data as possible is sent
> with the initial partial delivery and that following partial
> deliveries contain all available data.
> 
> In sctp_ulpq_partial_delivery(), attempt partial delivery only
> if the data on the head of the reassembly queue is at or before
> the cumulative TSN ACK point.
> 
> In sctp_ulpq_renege(), use the modified return values from
> sctp_ulpq_tail_data() to choose whether to attempt partial
> delivery or to attempt to drain the reassembly queue as a
> means to reduce memory pressure.  Remove call to
> sctp_tsnmap_mark(), as this is handled correctly in call to
> sctp_ulpq_tail_data().
> 
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 

[PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Roberts, Lee A.]

From: Lee A. Roberts <lee.roberts@hp.com>

Resolve SCTP association hangs observed during SCTP stress
testing.  Observable symptoms include communications hangs
with data being held in the association lobby (ordering)
queue.  Close examination of reassembly/ordering queues shows
duplicated packets.

In sctp_tsnmap_grow(), correct off-by-one errors when copying
and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
word, this bit can be lost, causing the corresponding packet
to be transmitted again and to be entered as a duplicate into
the SCTP reassembly/ordering queues.

Patch applies to linux-3.8 kernel.

Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
 net/sctp/tsnmap.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff -uprN -X linux-3.8-vanilla/Documentation/dontdiff linux-3.8-vanilla/net/sctp/tsnmap.c linux-3.8-SCTP+1/net/sctp/tsnmap.c
--- linux-3.8-vanilla/net/sctp/tsnmap.c	2013-02-18 16:58:34.000000000 -0700
+++ linux-3.8-SCTP+1/net/sctp/tsnmap.c	2013-02-20 08:01:02.555223259 -0700
@@ -369,14 +369,15 @@ static int sctp_tsnmap_grow(struct sctp_
 	if (gap >= SCTP_TSN_MAP_SIZE)
 		return 0;
 
-	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
+	inc = ALIGN((gap - map->len + 1), BITS_PER_LONG)
+		+ SCTP_TSN_MAP_INCREMENT;
 	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
 
 	new = kzalloc(len>>3, GFP_ATOMIC);
 	if (!new)
 		return 0;
 
-	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
+	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn + 1);
 	kfree(map->tsn_map);
 	map->tsn_map = new;
 	map->len = len;

Re: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Vlad Yasevich]

On 02/21/2013 11:44 AM, Roberts, Lee A. wrote:
> From: Lee A. Roberts <lee.roberts@hp.com>
>
> Resolve SCTP association hangs observed during SCTP stress
> testing.  Observable symptoms include communications hangs
> with data being held in the association lobby (ordering)
> queue.  Close examination of reassembly/ordering queues shows
> duplicated packets.
>
> In sctp_tsnmap_grow(), correct off-by-one errors when copying
> and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
> word, this bit can be lost, causing the corresponding packet
> to be transmitted again and to be entered as a duplicate into
> the SCTP reassembly/ordering queues.
>
> Patch applies to linux-3.8 kernel.
>
> Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
> ---
>   net/sctp/tsnmap.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff -uprN -X linux-3.8-vanilla/Documentation/dontdiff linux-3.8-vanilla/net/sctp/tsnmap.c linux-3.8-SCTP+1/net/sctp/tsnmap.c
> --- linux-3.8-vanilla/net/sctp/tsnmap.c	2013-02-18 16:58:34.000000000 -0700
> +++ linux-3.8-SCTP+1/net/sctp/tsnmap.c	2013-02-20 08:01:02.555223259 -0700
> @@ -369,14 +369,15 @@ static int sctp_tsnmap_grow(struct sctp_
>   	if (gap >= SCTP_TSN_MAP_SIZE)

No that I think about this a bit more, this should be gap + 1.  If you 
do that, you might as well call sctp_tsnmap_grow() with gap+1 as 
argument and then can just use the 'gap' everywhere inside.

>   		return 0;
>
> -	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
> +	inc = ALIGN((gap - map->len + 1), BITS_PER_LONG)
> +		+ SCTP_TSN_MAP_INCREMENT;
>   	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
>
>   	new = kzalloc(len>>3, GFP_ATOMIC);
>   	if (!new)
>   		return 0;
>
> -	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
> +	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn + 1);

Can simplify that this by using map->cumulative_tsn_ack_point instead of 
base_tsn.

-vlad

>   	kfree(map->tsn_map);
>   	map->tsn_map = new;
>   	map->len = len;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Roberts, Lee A.]

Vlad,

> -----Original Message-----
> From: Vlad Yasevich [mailto:vyasevich@gmail.com]
> Sent: Thursday, February 21, 2013 10:19 AM
> To: Roberts, Lee A.
> Cc: linux-sctp@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()
> 
> On 02/21/2013 11:44 AM, Roberts, Lee A. wrote:
> > From: Lee A. Roberts <lee.roberts@hp.com>
> >
> > Resolve SCTP association hangs observed during SCTP stress
> > testing.  Observable symptoms include communications hangs
> > with data being held in the association lobby (ordering)
> > queue.  Close examination of reassembly/ordering queues shows
> > duplicated packets.
> >
> > In sctp_tsnmap_grow(), correct off-by-one errors when copying
> > and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
> > word, this bit can be lost, causing the corresponding packet
> > to be transmitted again and to be entered as a duplicate into
> > the SCTP reassembly/ordering queues.
> >
> > Patch applies to linux-3.8 kernel.
> >
> > Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
> > ---
> >   net/sctp/tsnmap.c |    5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff -uprN -X linux-3.8-vanilla/Documentation/dontdiff linux-3.8-vanilla/net/sctp/tsnmap.c linux-
> 3.8-SCTP+1/net/sctp/tsnmap.c
> > --- linux-3.8-vanilla/net/sctp/tsnmap.c	2013-02-18 16:58:34.000000000 -0700
> > +++ linux-3.8-SCTP+1/net/sctp/tsnmap.c	2013-02-20 08:01:02.555223259 -0700
> > @@ -369,14 +369,15 @@ static int sctp_tsnmap_grow(struct sctp_
> >   	if (gap >= SCTP_TSN_MAP_SIZE)
> 
> No that I think about this a bit more, this should be gap + 1.  If you
> do that, you might as well call sctp_tsnmap_grow() with gap+1 as
> argument and then can just use the 'gap' everywhere inside.

I think the calculation of "gap" in sctp_tsnmap_mark() should change:

-	gap = tsn - map->base_tsn;
+	gap = tsn - map->cumulative_tsn_ack_point;

> 
> >   		return 0;
> >
> > -	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
> > +	inc = ALIGN((gap - map->len + 1), BITS_PER_LONG)
> > +		+ SCTP_TSN_MAP_INCREMENT;
> >   	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
> >
> >   	new = kzalloc(len>>3, GFP_ATOMIC);
> >   	if (!new)
> >   		return 0;
> >
> > -	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
> > +	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn + 1);
> 
> Can simplify that this by using map->cumulative_tsn_ack_point instead of
> base_tsn.
> 
> -vlad
> 

I changed the code to use "cumulative_tsn_ack_point" in an updated version
of the patch.

                                         -- Lee

> >   	kfree(map->tsn_map);
> >   	map->tsn_map = new;
> >   	map->len = len;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

RE: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()

[Roberts, Lee A.]

Vlad,

> -----Original Message-----
> From: Roberts, Lee A.
> Sent: Thursday, February 21, 2013 11:00 AM
> To: 'Vlad Yasevich'
> Cc: linux-sctp@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()
> 
> Vlad,
> 
> > -----Original Message-----
> > From: Vlad Yasevich [mailto:vyasevich@gmail.com]
> > Sent: Thursday, February 21, 2013 10:19 AM
> > To: Roberts, Lee A.
> > Cc: linux-sctp@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()
> >
> > On 02/21/2013 11:44 AM, Roberts, Lee A. wrote:
> > > From: Lee A. Roberts <lee.roberts@hp.com>
> > >
> > > Resolve SCTP association hangs observed during SCTP stress
> > > testing.  Observable symptoms include communications hangs
> > > with data being held in the association lobby (ordering)
> > > queue.  Close examination of reassembly/ordering queues shows
> > > duplicated packets.
> > >
> > > In sctp_tsnmap_grow(), correct off-by-one errors when copying
> > > and resizing the tsnmap.  If max_tsn_seen is in the LSB of the
> > > word, this bit can be lost, causing the corresponding packet
> > > to be transmitted again and to be entered as a duplicate into
> > > the SCTP reassembly/ordering queues.
> > >
> > > Patch applies to linux-3.8 kernel.
> > >
> > > Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
> > > ---
> > >   net/sctp/tsnmap.c |    5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff -uprN -X linux-3.8-vanilla/Documentation/dontdiff linux-3.8-vanilla/net/sctp/tsnmap.c linux-
> > 3.8-SCTP+1/net/sctp/tsnmap.c
> > > --- linux-3.8-vanilla/net/sctp/tsnmap.c	2013-02-18 16:58:34.000000000 -0700
> > > +++ linux-3.8-SCTP+1/net/sctp/tsnmap.c	2013-02-20 08:01:02.555223259 -0700
> > > @@ -369,14 +369,15 @@ static int sctp_tsnmap_grow(struct sctp_
> > >   	if (gap >= SCTP_TSN_MAP_SIZE)
> >
> > No that I think about this a bit more, this should be gap + 1.  If you
> > do that, you might as well call sctp_tsnmap_grow() with gap+1 as
> > argument and then can just use the 'gap' everywhere inside.
> 
> I think the calculation of "gap" in sctp_tsnmap_mark() should change:
> 
> -	gap = tsn - map->base_tsn;
> +	gap = tsn - map->cumulative_tsn_ack_point;
> 

Oops, this breaks the logic that follows.  Using "gap + 1" in the call should work.
I sent an updated patch (v3).

                                    - Lee

> >
> > >   		return 0;
> > >
> > > -	inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
> > > +	inc = ALIGN((gap - map->len + 1), BITS_PER_LONG)
> > > +		+ SCTP_TSN_MAP_INCREMENT;
> > >   	len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
> > >
> > >   	new = kzalloc(len>>3, GFP_ATOMIC);
> > >   	if (!new)
> > >   		return 0;
> > >
> > > -	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
> > > +	bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn + 1);
> >
> > Can simplify that this by using map->cumulative_tsn_ack_point instead of
> > base_tsn.
> >
> > -vlad
> >
> 
> I changed the code to use "cumulative_tsn_ack_point" in an updated version
> of the patch.
> 
>                                          -- Lee
> 
> > >   	kfree(map->tsn_map);
> > >   	map->tsn_map = new;
> > >   	map->len = len;
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >