[SCTP]: Allow spillover of receive buffer to avoid deadlock.

This patch fixes a deadlock situation in the receive path by allowing temporary spillover of the receive buffer. - If the chunk we receive has a tsn that immediately follows the ctsn, accept it even if we run out of receive buffer space and renege data with higher TSNs. - Once we accept one chunk in a packet, accept all the remaining chunks even if we run out of receive buffer space. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Acked-by: Mark Butler <butlerm@middle.net> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com> Signed-off-by: Sridhar Samudrala <sri@us.ibm.com> Signed-off-by: David S. Miller <davem@davemloft.net>

[SCTP]: Allow spillover of receive buffer to avoid deadlock.
This patch fixes a deadlock situation in the receive path by allowing temporary spillover of the receive buffer. - If the chunk we receive has a tsn that immediately follows the ctsn, accept it even if we run out of receive buffer space and renege data with higher TSNs. - Once we accept one chunk in a packet, accept all the remaining chunks even if we run out of receive buffer space. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Acked-by: Mark Butler <butlerm@middle.net> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com> Signed-off-by: Sridhar Samudrala <sri@us.ibm.com> Signed-off-by: David S. Miller <davem@davemloft.net>
Neil Horman · David S. Miller
1 parent 91ef5d2d6e
Showing 3 changed files with 38 additions and 10 deletions Side-by-side Diff
include/net/sctp/structs.h
net/sctp/inqueue.c
net/sctp/sm_statefuns.c
@@ -712,6 +712,7 @@
 	__u8 tsn_gap_acked;	/* Is this chunk acked by a GAP ACK? */
 	__s8 fast_retransmit;	 /* Is this chunk fast retransmitted? */
 	__u8 tsn_missing_report; /* Data chunk missing counter. */
+	__u8 data_accepted; 	/* At least 1 chunk in this packet accepted */
 };
  
 void sctp_chunk_hold(struct sctp_chunk *);
@@ -149,6 +149,7 @@
 		/* This is the first chunk in the packet.  */
 		chunk->singleton = 1;
 		ch = (sctp_chunkhdr_t *) chunk->skb->data;
+		chunk->data_accepted = 0;
 	}
  
         chunk->chunk_hdr = ch;
@@ -5151,7 +5151,9 @@
 	int tmp;
 	__u32 tsn;
 	int account_value;
+	struct sctp_tsnmap *map = (struct sctp_tsnmap *)&asoc->peer.tsn_map;
 	struct sock *sk = asoc->base.sk;
+	int rcvbuf_over = 0;
  
 	data_hdr = chunk->subh.data_hdr = (sctp_datahdr_t *)chunk->skb->data;
 	skb_pull(chunk->skb, sizeof(sctp_datahdr_t));
  
@@ -5162,10 +5164,16 @@
 	/* ASSERT:  Now skb->data is really the user data.  */
  
 	/*
-	 * if we are established, and we have used up our receive
-	 * buffer memory, drop the frame
+	 * If we are established, and we have used up our receive buffer
+	 * memory, think about droping the frame.
+	 * Note that we have an opportunity to improve performance here.
+	 * If we accept one chunk from an skbuff, we have to keep all the
+	 * memory of that skbuff around until the chunk is read into user
+	 * space. Therefore, once we accept 1 chunk we may as well accept all
+	 * remaining chunks in the skbuff. The data_accepted flag helps us do
+	 * that.
 	 */
-	if (asoc->state == SCTP_STATE_ESTABLISHED) {
+	if ((asoc->state == SCTP_STATE_ESTABLISHED) && (!chunk->data_accepted)) {
 		/*
 		 * If the receive buffer policy is 1, then each
 		 * association can allocate up to sk_rcvbuf bytes
  
@@ -5176,9 +5184,25 @@
 			account_value = atomic_read(&asoc->rmem_alloc);
 		else
 			account_value = atomic_read(&sk->sk_rmem_alloc);
+		if (account_value > sk->sk_rcvbuf) {
+			/*
+			 * We need to make forward progress, even when we are
+			 * under memory pressure, so we always allow the
+			 * next tsn after the ctsn ack point to be accepted.
+			 * This lets us avoid deadlocks in which we have to
+			 * drop frames that would otherwise let us drain the
+			 * receive queue.
+			 */
+			if ((sctp_tsnmap_get_ctsn(map) + 1) != tsn)
+				return SCTP_IERROR_IGNORE_TSN;
  
-		if (account_value > sk->sk_rcvbuf)
-			return SCTP_IERROR_IGNORE_TSN;
+			/*
+			 * We're going to accept the frame but we should renege
+			 * to make space for it. This will send us down that
+			 * path later in this function.
+			 */
+			rcvbuf_over = 1;
+		}
 	}
  
 	/* Process ECN based congestion.
@@ -5226,6 +5250,7 @@
 	datalen -= sizeof(sctp_data_chunk_t);
  
 	deliver = SCTP_CMD_CHUNK_ULP;
+	chunk->data_accepted = 1;
  
 	/* Think about partial delivery. */
 	if ((datalen >= asoc->rwnd) && (!asoc->ulpq.pd_mode)) {
@@ -5242,7 +5267,8 @@
 	 * large spill over.
 	 */
 	if (!asoc->rwnd || asoc->rwnd_over ||
-	    (datalen > asoc->rwnd + asoc->frag_point)) {
+	    (datalen > asoc->rwnd + asoc->frag_point) ||
+	    rcvbuf_over) {
  
 		/* If this is the next TSN, consider reneging to make
 		 * room.   Note: Playing nice with a confused sender.  A
@@ -5250,8 +5276,8 @@
 		 * space and in the future we may want to detect and
 		 * do more drastic reneging.
 		 */
-		if (sctp_tsnmap_has_gap(&asoc->peer.tsn_map) &&
-		    (sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1) == tsn) {
+		if (sctp_tsnmap_has_gap(map) &&
+		    (sctp_tsnmap_get_ctsn(map) + 1) == tsn) {
 			SCTP_DEBUG_PRINTK("Reneging for tsn:%u\n", tsn);
 			deliver = SCTP_CMD_RENEGE;
 		} else {
...	...	@@ -712,6 +712,7 @@
712	712	__u8 tsn_gap_acked; /* Is this chunk acked by a GAP ACK? */
713	713	__s8 fast_retransmit; /* Is this chunk fast retransmitted? */
714	714	__u8 tsn_missing_report; /* Data chunk missing counter. */
	715	+ __u8 data_accepted; /* At least 1 chunk in this packet accepted */
715	716	};
716	717
717	718	void sctp_chunk_hold(struct sctp_chunk *);
...	...	@@ -149,6 +149,7 @@
149	149	/* This is the first chunk in the packet. */
150	150	chunk->singleton = 1;
151	151	ch = (sctp_chunkhdr_t *) chunk->skb->data;
	152	+ chunk->data_accepted = 0;
152	153	}
153	154
154	155	chunk->chunk_hdr = ch;
...	...	@@ -5151,7 +5151,9 @@
5151	5151	int tmp;
5152	5152	__u32 tsn;
5153	5153	int account_value;
	5154	+ struct sctp_tsnmap map = (struct sctp_tsnmap )&asoc->peer.tsn_map;
5154	5155	struct sock *sk = asoc->base.sk;
	5156	+ int rcvbuf_over = 0;
5155	5157
5156	5158	data_hdr = chunk->subh.data_hdr = (sctp_datahdr_t *)chunk->skb->data;
5157	5159	skb_pull(chunk->skb, sizeof(sctp_datahdr_t));
5158	5160
...	...	@@ -5162,10 +5164,16 @@
5162	5164	/* ASSERT: Now skb->data is really the user data. */
5163	5165
5164	5166	/*
5165		- * if we are established, and we have used up our receive
5166		- * buffer memory, drop the frame
	5167	+ * If we are established, and we have used up our receive buffer
	5168	+ * memory, think about droping the frame.
	5169	+ * Note that we have an opportunity to improve performance here.
	5170	+ * If we accept one chunk from an skbuff, we have to keep all the
	5171	+ * memory of that skbuff around until the chunk is read into user
	5172	+ * space. Therefore, once we accept 1 chunk we may as well accept all
	5173	+ * remaining chunks in the skbuff. The data_accepted flag helps us do
	5174	+ * that.
5167	5175	*/
5168		- if (asoc->state == SCTP_STATE_ESTABLISHED) {
	5176	+ if ((asoc->state == SCTP_STATE_ESTABLISHED) && (!chunk->data_accepted)) {
5169	5177	/*
5170	5178	* If the receive buffer policy is 1, then each
5171	5179	* association can allocate up to sk_rcvbuf bytes
5172	5180
...	...	@@ -5176,9 +5184,25 @@
5176	5184	account_value = atomic_read(&asoc->rmem_alloc);
5177	5185	else
5178	5186	account_value = atomic_read(&sk->sk_rmem_alloc);
	5187	+ if (account_value > sk->sk_rcvbuf) {
	5188	+ /*
	5189	+ * We need to make forward progress, even when we are
	5190	+ * under memory pressure, so we always allow the
	5191	+ * next tsn after the ctsn ack point to be accepted.
	5192	+ * This lets us avoid deadlocks in which we have to
	5193	+ * drop frames that would otherwise let us drain the
	5194	+ * receive queue.
	5195	+ */
	5196	+ if ((sctp_tsnmap_get_ctsn(map) + 1) != tsn)
	5197	+ return SCTP_IERROR_IGNORE_TSN;
5179	5198
5180		- if (account_value > sk->sk_rcvbuf)
5181		- return SCTP_IERROR_IGNORE_TSN;
	5199	+ /*
	5200	+ * We're going to accept the frame but we should renege
	5201	+ * to make space for it. This will send us down that
	5202	+ * path later in this function.
	5203	+ */
	5204	+ rcvbuf_over = 1;
	5205	+ }
5182	5206	}
5183	5207
5184	5208	/* Process ECN based congestion.
...	...	@@ -5226,6 +5250,7 @@
5226	5250	datalen -= sizeof(sctp_data_chunk_t);
5227	5251
5228	5252	deliver = SCTP_CMD_CHUNK_ULP;
	5253	+ chunk->data_accepted = 1;
5229	5254
5230	5255	/* Think about partial delivery. */
5231	5256	if ((datalen >= asoc->rwnd) && (!asoc->ulpq.pd_mode)) {
...	...	@@ -5242,7 +5267,8 @@
5242	5267	* large spill over.
5243	5268	*/
5244	5269	if (!asoc->rwnd \|\| asoc->rwnd_over \|\|
5245		- (datalen > asoc->rwnd + asoc->frag_point)) {
	5270	+ (datalen > asoc->rwnd + asoc->frag_point) \|\|
	5271	+ rcvbuf_over) {
5246	5272
5247	5273	/* If this is the next TSN, consider reneging to make
5248	5274	* room. Note: Playing nice with a confused sender. A
...	...	@@ -5250,8 +5276,8 @@
5250	5276	* space and in the future we may want to detect and
5251	5277	* do more drastic reneging.
5252	5278	*/
5253		- if (sctp_tsnmap_has_gap(&asoc->peer.tsn_map) &&
5254		- (sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1) == tsn) {
	5279	+ if (sctp_tsnmap_has_gap(map) &&
	5280	+ (sctp_tsnmap_get_ctsn(map) + 1) == tsn) {
5255	5281	SCTP_DEBUG_PRINTK("Reneging for tsn:%u\n", tsn);
5256	5282	deliver = SCTP_CMD_RENEGE;
5257	5283	} else {