[PATCH net 0/6] sctp: fix the transmit err process

[PATCH net 0/6] sctp: fix the transmit err process

[Xin Long]

This patchset is to improve the transmit err process and also fix some
issues.

After this patchset, once the chunks are enqueued successfully, even
if the chunks fail to send out, no matter because of nodst or nomem,
no err retruns back to users any more. Instead, they are taken care
of by retransmit.

Xin Long (6):
  sctp: remove the unnecessary state check in sctp_outq_tail
  sctp: do not return the transmit err back to sctp_sendmsg
  sctp: free msg->chunks when sctp_primitive_SEND return err
  sctp: save transmit error to sk_err in sctp_outq_flush
  sctp: make sctp_outq_flush/tail/uncork return void
  sctp: not return ENOMEM err back in sctp_packet_transmit

 include/net/sctp/structs.h |  5 +--
 net/sctp/chunk.c           | 13 +++++++
 net/sctp/output.c          | 50 +++++++++++++-------------
 net/sctp/outqueue.c        | 89 ++++++++++++++++------------------------------
 net/sctp/sm_sideeffect.c   | 25 +++++--------
 net/sctp/socket.c          |  6 ++--
 6 files changed, 84 insertions(+), 104 deletions(-)

-- 
2.1.0

[PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Xin Long]

Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks
the asoc's state through statetable before calling sctp_outq_tail. So
there's no need to do it again in sctp_outq_tail.

This patch is to remove it from sctp_outq_tail.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/outqueue.c | 53 ++++++++++++++---------------------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 72e54a4..da2418b 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -299,50 +299,25 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp)
 	 * immediately.
 	 */
 	if (sctp_chunk_is_data(chunk)) {
-		/* Is it OK to queue data chunks?  */
-		/* From 9. Termination of Association
-		 *
-		 * When either endpoint performs a shutdown, the
-		 * association on each peer will stop accepting new
-		 * data from its user and only deliver data in queue
-		 * at the time of sending or receiving the SHUTDOWN
-		 * chunk.
-		 */
-		switch (q->asoc->state) {
-		case SCTP_STATE_CLOSED:
-		case SCTP_STATE_SHUTDOWN_PENDING:
-		case SCTP_STATE_SHUTDOWN_SENT:
-		case SCTP_STATE_SHUTDOWN_RECEIVED:
-		case SCTP_STATE_SHUTDOWN_ACK_SENT:
-			/* Cannot send after transport endpoint shutdown */
-			error = -ESHUTDOWN;
-			break;
-
-		default:
-			pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n",
-				 __func__, q, chunk, chunk && chunk->chunk_hdr ?
-				 sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) :
-				 "illegal chunk");
-
-			sctp_chunk_hold(chunk);
-			sctp_outq_tail_data(q, chunk);
-			if (chunk->asoc->prsctp_enable &&
-			    SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags))
-				chunk->asoc->sent_cnt_removable++;
-			if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
-				SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS);
-			else
-				SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS);
-			break;
-		}
+		pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n",
+			 __func__, q, chunk, chunk && chunk->chunk_hdr ?
+			 sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) :
+			 "illegal chunk");
+
+		sctp_chunk_hold(chunk);
+		sctp_outq_tail_data(q, chunk);
+		if (chunk->asoc->prsctp_enable &&
+		    SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags))
+			chunk->asoc->sent_cnt_removable++;
+		if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
+			SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS);
+		else
+			SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS);
 	} else {
 		list_add_tail(&chunk->list, &q->control_chunk_list);
 		SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
 	}
 
-	if (error < 0)
-		return error;
-
 	if (!q->cork)
 		error = sctp_outq_flush(q, 0, gfp);
 
-- 
2.1.0

[PATCH net 2/6] sctp: do not return the transmit err back to sctp_sendmsg

[Xin Long]

Once a chunk is enqueued successfully, sctp queues can take care of it.
Even if it is failed to transmit (like because of nomem), it should be
put into retransmit queue.

If sctp report this error to users, it confuses them, they may resend
that msg, but actually in kernel sctp stack is in charge of retransmit
it already.

Besides, this error probably is not from the failure of transmitting
current msg, but transmitting or retransmitting another msg's chunks,
as sctp_outq_flush just tries to send out all transports' chunks.

This patch is to make sctp_cmd_send_msg return avoid, and not return the
transmit err back to sctp_sendmsg

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_sideeffect.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 12d4519..cf6e4f0 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -1020,19 +1020,13 @@ static void sctp_cmd_t1_timer_update(struct sctp_association *asoc,
  * This way the whole message is queued up and bundling if
  * encouraged for small fragments.
  */
-static int sctp_cmd_send_msg(struct sctp_association *asoc,
-				struct sctp_datamsg *msg, gfp_t gfp)
+static void sctp_cmd_send_msg(struct sctp_association *asoc,
+			      struct sctp_datamsg *msg, gfp_t gfp)
 {
 	struct sctp_chunk *chunk;
-	int error = 0;
-
-	list_for_each_entry(chunk, &msg->chunks, frag_list) {
-		error = sctp_outq_tail(&asoc->outqueue, chunk, gfp);
-		if (error)
-			break;
-	}
 
-	return error;
+	list_for_each_entry(chunk, &msg->chunks, frag_list)
+		sctp_outq_tail(&asoc->outqueue, chunk, gfp);
 }
 
 
@@ -1709,7 +1703,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				sctp_outq_cork(&asoc->outqueue);
 				local_cork = 1;
 			}
-			error = sctp_cmd_send_msg(asoc, cmd->obj.msg, gfp);
+			sctp_cmd_send_msg(asoc, cmd->obj.msg, gfp);
 			break;
 		case SCTP_CMD_SEND_NEXT_ASCONF:
 			sctp_cmd_send_asconf(asoc);
-- 
2.1.0

[PATCH net 3/6] sctp: free msg->chunks when sctp_primitive_SEND return err

[Xin Long]

Last patch "sctp: do not return the transmit err back to sctp_sendmsg"
made sctp_primitive_SEND return err only when asoc state is unavailable.
In this case, chunks are not enqueued, they have no chance to be freed if
we don't take care of them later.

This Patch is actually to revert commit 1cd4d5c4326a ("sctp: remove the
unused sctp_datamsg_free()") and commit 8b570dc9f7b6 ("sctp: only drop the
reference on the datamsg after sending a msg"), to use sctp_datamsg_free to
free the chunks of current msg.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  1 +
 net/sctp/chunk.c           | 13 +++++++++++++
 net/sctp/socket.c          |  6 ++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index ce93c4b..f61fb7c 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -537,6 +537,7 @@ struct sctp_datamsg {
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
 					    struct sctp_sndrcvinfo *,
 					    struct iov_iter *);
+void sctp_datamsg_free(struct sctp_datamsg *);
 void sctp_datamsg_put(struct sctp_datamsg *);
 void sctp_chunk_fail(struct sctp_chunk *, int error);
 int sctp_chunk_abandoned(struct sctp_chunk *);
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index a55e547..af9cc80 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -70,6 +70,19 @@ static struct sctp_datamsg *sctp_datamsg_new(gfp_t gfp)
 	return msg;
 }
 
+void sctp_datamsg_free(struct sctp_datamsg *msg)
+{
+	struct sctp_chunk *chunk;
+
+	/* This doesn't have to be a _safe vairant because
+	 * sctp_chunk_free() only drops the refs.
+	 */
+	list_for_each_entry(chunk, &msg->chunks, frag_list)
+		sctp_chunk_free(chunk);
+
+	sctp_datamsg_put(msg);
+}
+
 /* Final destructruction of datamsg memory. */
 static void sctp_datamsg_destroy(struct sctp_datamsg *msg)
 {
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9fc417a..9d8e2be 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1970,13 +1970,15 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
 	 * breaks.
 	 */
 	err = sctp_primitive_SEND(net, asoc, datamsg);
-	sctp_datamsg_put(datamsg);
 	/* Did the lower layer accept the chunk? */
-	if (err)
+	if (err) {
+		sctp_datamsg_free(datamsg);
 		goto out_free;
+	}
 
 	pr_debug("%s: we sent primitively\n", __func__);
 
+	sctp_datamsg_put(datamsg);
 	err = msg_len;
 
 	if (unlikely(wait_connect)) {
-- 
2.1.0

Re: [PATCH net 3/6] sctp: free msg->chunks when sctp_primitive_SEND return err

[Marcelo Ricardo Leitner]

On Thu, Sep 08, 2016 at 05:31:47PM +0800, Xin Long wrote:
> Last patch "sctp: do not return the transmit err back to sctp_sendmsg"
> made sctp_primitive_SEND return err only when asoc state is unavailable.
> In this case, chunks are not enqueued, they have no chance to be freed if
> we don't take care of them later.
> 
> This Patch is actually to revert commit 1cd4d5c4326a ("sctp: remove the
> unused sctp_datamsg_free()") and commit 8b570dc9f7b6 ("sctp: only drop the
> reference on the datamsg after sending a msg"), to use sctp_datamsg_free to
> free the chunks of current msg.
> 

Considering the subsequent patches in this series are improving error
return and the first is a cleanup/optimization, Xin, this patch and the
previous one both deserve a Fixes tag too, for 8b570dc9f7b6. Thanks

> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/structs.h |  1 +
>  net/sctp/chunk.c           | 13 +++++++++++++
>  net/sctp/socket.c          |  6 ++++--
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index ce93c4b..f61fb7c 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -537,6 +537,7 @@ struct sctp_datamsg {
>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>  					    struct sctp_sndrcvinfo *,
>  					    struct iov_iter *);
> +void sctp_datamsg_free(struct sctp_datamsg *);
>  void sctp_datamsg_put(struct sctp_datamsg *);
>  void sctp_chunk_fail(struct sctp_chunk *, int error);
>  int sctp_chunk_abandoned(struct sctp_chunk *);
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index a55e547..af9cc80 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -70,6 +70,19 @@ static struct sctp_datamsg *sctp_datamsg_new(gfp_t gfp)
>  	return msg;
>  }
>  
> +void sctp_datamsg_free(struct sctp_datamsg *msg)
> +{
> +	struct sctp_chunk *chunk;
> +
> +	/* This doesn't have to be a _safe vairant because
> +	 * sctp_chunk_free() only drops the refs.
> +	 */
> +	list_for_each_entry(chunk, &msg->chunks, frag_list)
> +		sctp_chunk_free(chunk);
> +
> +	sctp_datamsg_put(msg);
> +}
> +
>  /* Final destructruction of datamsg memory. */
>  static void sctp_datamsg_destroy(struct sctp_datamsg *msg)
>  {
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 9fc417a..9d8e2be 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1970,13 +1970,15 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  	 * breaks.
>  	 */
>  	err = sctp_primitive_SEND(net, asoc, datamsg);
> -	sctp_datamsg_put(datamsg);
>  	/* Did the lower layer accept the chunk? */
> -	if (err)
> +	if (err) {
> +		sctp_datamsg_free(datamsg);
>  		goto out_free;
> +	}
>  
>  	pr_debug("%s: we sent primitively\n", __func__);
>  
> +	sctp_datamsg_put(datamsg);
>  	err = msg_len;
>  
>  	if (unlikely(wait_connect)) {
> -- 
> 2.1.0
> 
> --
> 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 net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Neil Horman]

On Thu, Sep 08, 2016 at 05:31:45PM +0800, Xin Long wrote:
> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks
> the asoc's state through statetable before calling sctp_outq_tail. So
> there's no need to do it again in sctp_outq_tail.
> 
> This patch is to remove it from sctp_outq_tail.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
This doesn't seem safe to me.  The send operation is handled in side effect
processing off a queue that might have operations queued ahead of it, which
affect the associations state.  I think you need to keep this check in place

Neil

> ---
>  net/sctp/outqueue.c | 53 ++++++++++++++---------------------------------------
>  1 file changed, 14 insertions(+), 39 deletions(-)
> 
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 72e54a4..da2418b 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -299,50 +299,25 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp)
>  	 * immediately.
>  	 */
>  	if (sctp_chunk_is_data(chunk)) {
> -		/* Is it OK to queue data chunks?  */
> -		/* From 9. Termination of Association
> -		 *
> -		 * When either endpoint performs a shutdown, the
> -		 * association on each peer will stop accepting new
> -		 * data from its user and only deliver data in queue
> -		 * at the time of sending or receiving the SHUTDOWN
> -		 * chunk.
> -		 */
> -		switch (q->asoc->state) {
> -		case SCTP_STATE_CLOSED:
> -		case SCTP_STATE_SHUTDOWN_PENDING:
> -		case SCTP_STATE_SHUTDOWN_SENT:
> -		case SCTP_STATE_SHUTDOWN_RECEIVED:
> -		case SCTP_STATE_SHUTDOWN_ACK_SENT:
> -			/* Cannot send after transport endpoint shutdown */
> -			error = -ESHUTDOWN;
> -			break;
> -
> -		default:
> -			pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n",
> -				 __func__, q, chunk, chunk && chunk->chunk_hdr ?
> -				 sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) :
> -				 "illegal chunk");
> -
> -			sctp_chunk_hold(chunk);
> -			sctp_outq_tail_data(q, chunk);
> -			if (chunk->asoc->prsctp_enable &&
> -			    SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags))
> -				chunk->asoc->sent_cnt_removable++;
> -			if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
> -				SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS);
> -			else
> -				SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS);
> -			break;
> -		}
> +		pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n",
> +			 __func__, q, chunk, chunk && chunk->chunk_hdr ?
> +			 sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) :
> +			 "illegal chunk");
> +
> +		sctp_chunk_hold(chunk);
> +		sctp_outq_tail_data(q, chunk);
> +		if (chunk->asoc->prsctp_enable &&
> +		    SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags))
> +			chunk->asoc->sent_cnt_removable++;
> +		if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
> +			SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS);
> +		else
> +			SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS);
>  	} else {
>  		list_add_tail(&chunk->list, &q->control_chunk_list);
>  		SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
>  	}
>  
> -	if (error < 0)
> -		return error;
> -
>  	if (!q->cork)
>  		error = sctp_outq_flush(q, 0, gfp);
>  
> -- 
> 2.1.0
> 
> --
> 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 net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Xin Long]

>> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks
>> the asoc's state through statetable before calling sctp_outq_tail. So
>> there's no need to do it again in sctp_outq_tail.
>>
>> This patch is to remove it from sctp_outq_tail.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> This doesn't seem safe to me.  The send operation is handled in side effect
> processing off a queue that might have operations queued ahead of it, which
> affect the associations state.  I think you need to keep this check in place
>
But not really for Data Chunk.

There are 3 places calling sctp_outq_tail.
1. in sctp_assoc_rwnd_increase():
   it's for sending sack chunk, so this patch is safe for here
2. sctp_cmd_interpreter(), SCTP_CMD_REPLY:
   still not Data Chunk, so safe.
3.SCTP_CMD_SEND_MSG: ->  sctp_cmd_send_msg()
   it's for Data Chunk, but it's only called by sctp_sf_do_prm_send().

#define TYPE_SCTP_PRIMITIVE_SEND  { \
...
/* SCTP_STATE_COOKIE_WAIT */ \
TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
/* SCTP_STATE_COOKIE_ECHOED */ \
TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
/* SCTP_STATE_ESTABLISHED */ \
TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
/* SCTP_STATE_SHUTDOWN_PENDING */ \


in sctp_sf_do_prm_send():
   sctp_add_cmd_sf(commands, SCTP_CMD_SEND_MSG, SCTP_DATAMSG(msg));
   return SCTP_DISPOSITION_CONSUME;

This function is called by PRIMITIVE_SEND, and every time after
calling sctp_do_sm(), there must be no operations inside.
so it's impossible that other operations queued ahead of it.

I'm not sure if I miss some special case, pls correct me if I'm wrong.

Thanks.

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Neil Horman]

On Fri, Sep 09, 2016 at 01:34:05AM +0800, Xin Long wrote:
> >> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks
> >> the asoc's state through statetable before calling sctp_outq_tail. So
> >> there's no need to do it again in sctp_outq_tail.
> >>
> >> This patch is to remove it from sctp_outq_tail.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > This doesn't seem safe to me.  The send operation is handled in side effect
> > processing off a queue that might have operations queued ahead of it, which
> > affect the associations state.  I think you need to keep this check in place
> >
> But not really for Data Chunk.
> 
> There are 3 places calling sctp_outq_tail.
> 1. in sctp_assoc_rwnd_increase():
>    it's for sending sack chunk, so this patch is safe for here
> 2. sctp_cmd_interpreter(), SCTP_CMD_REPLY:
>    still not Data Chunk, so safe.
> 3.SCTP_CMD_SEND_MSG: ->  sctp_cmd_send_msg()
>    it's for Data Chunk, but it's only called by sctp_sf_do_prm_send().
> 
Its not enough to just look at the paths where outq_tail is called, because
the outq_tail function is checking a state variable that can update
asynchronously in the side effect processing queue.  Look at any one of the
timer functions.  Those all fire asynchronous to the outq list, but they all
enqueue to the side effect list.  Its entirely possible that the state can be
ESTABLISHED in the primitive send path, but before you enqueue the SEND
sideffect, a timer will pop and enqueue a NEW_STATE side effect.  The end result
is you need to check the state again.

Neil

> #define TYPE_SCTP_PRIMITIVE_SEND  { \
> ...
> /* SCTP_STATE_COOKIE_WAIT */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_COOKIE_ECHOED */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_ESTABLISHED */ \
> TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \
> /* SCTP_STATE_SHUTDOWN_PENDING */ \
> 
> 
> in sctp_sf_do_prm_send():
>    sctp_add_cmd_sf(commands, SCTP_CMD_SEND_MSG, SCTP_DATAMSG(msg));
>    return SCTP_DISPOSITION_CONSUME;
> 
> This function is called by PRIMITIVE_SEND, and every time after
> calling sctp_do_sm(), there must be no operations inside.
> so it's impossible that other operations queued ahead of it.
> 
> I'm not sure if I miss some special case, pls correct me if I'm wrong.
> 
> Thanks.
> --
> 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 net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Xin Long]

>>
> Its not enough to just look at the paths where outq_tail is called, because
> the outq_tail function is checking a state variable that can update
> asynchronously in the side effect processing queue.  Look at any one of the
> timer functions.  Those all fire asynchronous to the outq list, but they all

Fortunately, sctp_do_sm is protected by lock_sock.
I just checked all the sctp_do_sm calling in  timer functions.

------------
        bh_lock_sock(sk);
        if (sock_owned_by_user(sk)) {    <----------- [1]
                pr_debug("%s: sock is busy\n", __func__);
                /* Try again later.  */
                mod_timer()
                goto out_unlock;
        }
        error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
out_unlock:
        bh_unlock_sock(sk);


which means sctp_do_sm is *non-reenterable* because of
sock lock's protection. even if it's interrupted by something
like timers, they have to delay their real process until sock
lock is released.

So It's still safe here. pls double-check.

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Neil Horman]

On Fri, Sep 09, 2016 at 03:11:57PM +0800, Xin Long wrote:
> >>
> > Its not enough to just look at the paths where outq_tail is called, because
> > the outq_tail function is checking a state variable that can update
> > asynchronously in the side effect processing queue.  Look at any one of the
> > timer functions.  Those all fire asynchronous to the outq list, but they all
> 
> Fortunately, sctp_do_sm is protected by lock_sock.
> I just checked all the sctp_do_sm calling in  timer functions.
> 
> ------------
>         bh_lock_sock(sk);
>         if (sock_owned_by_user(sk)) {    <----------- [1]
>                 pr_debug("%s: sock is busy\n", __func__);
>                 /* Try again later.  */
>                 mod_timer()
>                 goto out_unlock;
>         }
>         error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
> out_unlock:
>         bh_unlock_sock(sk);
> 
> 
> which means sctp_do_sm is *non-reenterable* because of
> sock lock's protection. even if it's interrupted by something
> like timers, they have to delay their real process until sock
> lock is released.
> 
> So It's still safe here. pls double-check.
> 
I don't know, I still don't feel safe about it.  I agree the socket lock keeps
the state from changing during a single transmission, which makes the use case
you are focused on correct.

That said, have you considered the retransmit case?  That is to say, if you
queue and flush the outq, and some packets fail delivery, and in the time
between the intial send and the expiration of the RTX timer (during which the
socket lock will have been released), an event may occur which changes the
transport state, which will then be ignored with your patch.

Neil

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Xin Long]

> I don't know, I still don't feel safe about it.  I agree the socket lock keeps
> the state from changing during a single transmission, which makes the use case
> you are focused on correct.
ok, :-)

>
> That said, have you considered the retransmit case?  That is to say, if you
> queue and flush the outq, and some packets fail delivery, and in the time
> between the intial send and the expiration of the RTX timer (during which the
> socket lock will have been released), an event may occur which changes the
> transport state, which will then be ignored with your patch.
Sorry, I'm not sure if I got it.

You mean "during which changes q->asoc->state", right ?

This patch removes the check of q->asoc->state in sctp_outq_tail().

sctp_outq_tail() is called for data only in:
sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg ->
sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail()

before calling sctp_primitive_SEND, hold sock lock first.
then sctp_primitive_SEND choose FUNC according:

#define TYPE_SCTP_PRIMITIVE_SEND  {
....

if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg,
but sctp_sf_error_closed/sctp_sf_error_shutdown,  sctp_outq_tail
can't be called, either.
I mean sctp_primitive_SEND do the same check for asoc->state
already actually.

so the code in sctp_outq_tail is redundant actually.




>
> Neil
>

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Marcelo Ricardo Leitner]

On Sat, Sep 10, 2016 at 12:03:53AM +0800, Xin Long wrote:
> > That said, have you considered the retransmit case?  That is to say, if you
> > queue and flush the outq, and some packets fail delivery, and in the time
> > between the intial send and the expiration of the RTX timer (during which the
> > socket lock will have been released), an event may occur which changes the
> > transport state, which will then be ignored with your patch.
> Sorry, I'm not sure if I got it.
> 
> You mean "during which changes q->asoc->state", right ?
> 
> This patch removes the check of q->asoc->state in sctp_outq_tail().
> 
> sctp_outq_tail() is called for data only in:
> sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg ->
> sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail()
> 
> before calling sctp_primitive_SEND, hold sock lock first.
> then sctp_primitive_SEND choose FUNC according:
> 
> #define TYPE_SCTP_PRIMITIVE_SEND  {
> ....
> 
> if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg,
> but sctp_sf_error_closed/sctp_sf_error_shutdown,  sctp_outq_tail
> can't be called, either.
> I mean sctp_primitive_SEND do the same check for asoc->state
> already actually.
> 
> so the code in sctp_outq_tail is redundant actually.

I also don't see an issue with this patch, btw.

Xin, you may want to add more/such details to the changelog, specially
about the timer versus primitive handling.

  Marcelo

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Xin Long]

>
> I also don't see an issue with this patch, btw.
>
> Xin, you may want to add more/such details to the changelog, specially
> about the timer versus primitive handling.
>
OK, I will post v2 of this patchset.

Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq_tail

[Neil Horman]

On Sat, Sep 10, 2016 at 12:03:53AM +0800, Xin Long wrote:
> > I don't know, I still don't feel safe about it.  I agree the socket lock keeps
> > the state from changing during a single transmission, which makes the use case
> > you are focused on correct.
> ok, :-)
> 
> >
> > That said, have you considered the retransmit case?  That is to say, if you
> > queue and flush the outq, and some packets fail delivery, and in the time
> > between the intial send and the expiration of the RTX timer (during which the
> > socket lock will have been released), an event may occur which changes the
> > transport state, which will then be ignored with your patch.
> Sorry, I'm not sure if I got it.
> 
> You mean "during which changes q->asoc->state", right ?
> 
> This patch removes the check of q->asoc->state in sctp_outq_tail().
> 
> sctp_outq_tail() is called for data only in:
> sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg ->
> sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail()
> 
> before calling sctp_primitive_SEND, hold sock lock first.
> then sctp_primitive_SEND choose FUNC according:
> 
> #define TYPE_SCTP_PRIMITIVE_SEND  {
> ....
> 
> if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg,
> but sctp_sf_error_closed/sctp_sf_error_shutdown,  sctp_outq_tail
> can't be called, either.
> I mean sctp_primitive_SEND do the same check for asoc->state
> already actually.
> 
> so the code in sctp_outq_tail is redundant actually.
> 
> 
> 
> 
> >
> > Neil
> >
> 

Ok, you've convinced me, thanks for taking the time to go through it

Acked-by: Neil Horman <nhorman@tuxdriver.com>

[PATCH net 4/6] sctp: save transmit error to sk_err in sctp_outq_flush

[Xin Long]

Every time when sctp calls sctp_outq_flush, it sends out the chunks of
control queue, retransmit queue and data queue. Even if some trunks are
failed to transmit, it still has to flush all the transports, as it's
the only chance to clean that transmit_list.

So the latest transmit error here should be returned back. This transmit
error is an internal error of sctp stack.

I checked all the places where it uses the transmit error (the return
value of sctp_outq_flush), most of them are actually just save it to
sk_err.

Except for sctp_assoc/endpoint_bh_rcv, they will drop the chunk if
it's failed to send a REPLY, which is actually incorrect, as we can't
be sure the error that sctp_outq_flush returns is from sending that
REPLY.

So it's meaningless for sctp_outq_flush to return error back.

This patch is to save transmit error to sk_err in sctp_outq_flush, the
new error can update the old value. Eventually, sctp_wait_for_* would
check for it.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/output.c   |  3 ++-
 net/sctp/outqueue.c | 21 ++++++++++++---------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/sctp/output.c b/net/sctp/output.c
index 1f1682b..1934933 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -180,7 +180,6 @@ sctp_xmit_t sctp_packet_transmit_chunk(struct sctp_packet *packet,
 				       int one_packet, gfp_t gfp)
 {
 	sctp_xmit_t retval;
-	int error = 0;
 
 	pr_debug("%s: packet:%p size:%Zu chunk:%p size:%d\n", __func__,
 		 packet, packet->size, chunk, chunk->skb ? chunk->skb->len : -1);
@@ -188,6 +187,8 @@ sctp_xmit_t sctp_packet_transmit_chunk(struct sctp_packet *packet,
 	switch ((retval = (sctp_packet_append_chunk(packet, chunk)))) {
 	case SCTP_XMIT_PMTU_FULL:
 		if (!packet->has_cookie_echo) {
+			int error = 0;
+
 			error = sctp_packet_transmit(packet, gfp);
 			if (error < 0)
 				chunk->skb->sk->sk_err = -error;
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index da2418b..0c6210d 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -534,7 +534,6 @@ void sctp_retransmit(struct sctp_outq *q, struct sctp_transport *transport,
 		     sctp_retransmit_reason_t reason)
 {
 	struct net *net = sock_net(q->asoc->base.sk);
-	int error = 0;
 
 	switch (reason) {
 	case SCTP_RTXR_T3_RTX:
@@ -578,10 +577,7 @@ void sctp_retransmit(struct sctp_outq *q, struct sctp_transport *transport,
 	 * will be flushed at the end.
 	 */
 	if (reason != SCTP_RTXR_FAST_RTX)
-		error = sctp_outq_flush(q, /* rtx_timeout */ 1, GFP_ATOMIC);
-
-	if (error)
-		q->asoc->base.sk->sk_err = -error;
+		sctp_outq_flush(q, /* rtx_timeout */ 1, GFP_ATOMIC);
 }
 
 /*
@@ -894,8 +890,10 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
 			sctp_packet_config(&singleton, vtag, 0);
 			sctp_packet_append_chunk(&singleton, chunk);
 			error = sctp_packet_transmit(&singleton, gfp);
-			if (error < 0)
-				return error;
+			if (error < 0) {
+				asoc->base.sk->sk_err = -error;
+				return 0;
+			}
 			break;
 
 		case SCTP_CID_ABORT:
@@ -993,6 +991,8 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
 		retran:
 			error = sctp_outq_flush_rtx(q, packet,
 						    rtx_timeout, &start_timer);
+			if (error < 0)
+				asoc->base.sk->sk_err = -error;
 
 			if (start_timer) {
 				sctp_transport_reset_t3_rtx(transport);
@@ -1167,14 +1167,17 @@ sctp_flush_out:
 						      struct sctp_transport,
 						      send_ready);
 		packet = &t->packet;
-		if (!sctp_packet_empty(packet))
+		if (!sctp_packet_empty(packet)) {
 			error = sctp_packet_transmit(packet, gfp);
+			if (error < 0)
+				asoc->base.sk->sk_err = -error;
+		}
 
 		/* Clear the burst limited state, if any */
 		sctp_transport_burst_reset(t);
 	}
 
-	return error;
+	return 0;
 }
 
 /* Update unack_data based on the incoming SACK chunk */
-- 
2.1.0

[PATCH net 5/6] sctp: make sctp_outq_flush/tail/uncork return void

[Xin Long]

sctp_outq_flush return value is meaningless now, this patch is
to make sctp_outq_flush return void, as well as sctp_outq_fail
and sctp_outq_uncork.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  4 ++--
 net/sctp/outqueue.c        | 19 +++++++------------
 net/sctp/sm_sideeffect.c   |  9 ++++-----
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index f61fb7c..8693dc4 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1077,7 +1077,7 @@ struct sctp_outq {
 void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
 void sctp_outq_teardown(struct sctp_outq *);
 void sctp_outq_free(struct sctp_outq*);
-int sctp_outq_tail(struct sctp_outq *, struct sctp_chunk *chunk, gfp_t);
+void sctp_outq_tail(struct sctp_outq *, struct sctp_chunk *chunk, gfp_t);
 int sctp_outq_sack(struct sctp_outq *, struct sctp_chunk *);
 int sctp_outq_is_empty(const struct sctp_outq *);
 void sctp_outq_restart(struct sctp_outq *);
@@ -1085,7 +1085,7 @@ void sctp_outq_restart(struct sctp_outq *);
 void sctp_retransmit(struct sctp_outq *, struct sctp_transport *,
 		     sctp_retransmit_reason_t);
 void sctp_retransmit_mark(struct sctp_outq *, struct sctp_transport *, __u8);
-int sctp_outq_uncork(struct sctp_outq *, gfp_t gfp);
+void sctp_outq_uncork(struct sctp_outq *, gfp_t gfp);
 void sctp_prsctp_prune(struct sctp_association *asoc,
 		       struct sctp_sndrcvinfo *sinfo, int msg_len);
 /* Uncork and flush an outqueue.  */
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 0c6210d..f02495e 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -68,7 +68,7 @@ static void sctp_mark_missing(struct sctp_outq *q,
 
 static void sctp_generate_fwdtsn(struct sctp_outq *q, __u32 sack_ctsn);
 
-static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp);
+static void sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp);
 
 /* Add data to the front of the queue. */
 static inline void sctp_outq_head_data(struct sctp_outq *q,
@@ -285,10 +285,9 @@ void sctp_outq_free(struct sctp_outq *q)
 }
 
 /* Put a new chunk in an sctp_outq.  */
-int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp)
+void sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp)
 {
 	struct net *net = sock_net(q->asoc->base.sk);
-	int error = 0;
 
 	pr_debug("%s: outq:%p, chunk:%p[%s]\n", __func__, q, chunk,
 		 chunk && chunk->chunk_hdr ?
@@ -319,9 +318,7 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp)
 	}
 
 	if (!q->cork)
-		error = sctp_outq_flush(q, 0, gfp);
-
-	return error;
+		sctp_outq_flush(q, 0, gfp);
 }
 
 /* Insert a chunk into the sorted list based on the TSNs.  The retransmit list
@@ -749,12 +746,12 @@ redo:
 }
 
 /* Cork the outqueue so queued chunks are really queued. */
-int sctp_outq_uncork(struct sctp_outq *q, gfp_t gfp)
+void sctp_outq_uncork(struct sctp_outq *q, gfp_t gfp)
 {
 	if (q->cork)
 		q->cork = 0;
 
-	return sctp_outq_flush(q, 0, gfp);
+	sctp_outq_flush(q, 0, gfp);
 }
 
 
@@ -767,7 +764,7 @@ int sctp_outq_uncork(struct sctp_outq *q, gfp_t gfp)
  * locking concerns must be made.  Today we use the sock lock to protect
  * this function.
  */
-static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
+static void sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
 {
 	struct sctp_packet *packet;
 	struct sctp_packet singleton;
@@ -892,7 +889,7 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout, gfp_t gfp)
 			error = sctp_packet_transmit(&singleton, gfp);
 			if (error < 0) {
 				asoc->base.sk->sk_err = -error;
-				return 0;
+				return;
 			}
 			break;
 
@@ -1176,8 +1173,6 @@ sctp_flush_out:
 		/* Clear the burst limited state, if any */
 		sctp_transport_burst_reset(t);
 	}
-
-	return 0;
 }
 
 /* Update unack_data based on the incoming SACK chunk */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index cf6e4f0..c345bf1 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -1421,8 +1421,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				local_cork = 1;
 			}
 			/* Send a chunk to our peer.  */
-			error = sctp_outq_tail(&asoc->outqueue, cmd->obj.chunk,
-					       gfp);
+			sctp_outq_tail(&asoc->outqueue, cmd->obj.chunk, gfp);
 			break;
 
 		case SCTP_CMD_SEND_PKT:
@@ -1676,7 +1675,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 		case SCTP_CMD_FORCE_PRIM_RETRAN:
 			t = asoc->peer.retran_path;
 			asoc->peer.retran_path = asoc->peer.primary_path;
-			error = sctp_outq_uncork(&asoc->outqueue, gfp);
+			sctp_outq_uncork(&asoc->outqueue, gfp);
 			local_cork = 0;
 			asoc->peer.retran_path = t;
 			break;
@@ -1733,9 +1732,9 @@ out:
 	 */
 	if (asoc && SCTP_EVENT_T_CHUNK == event_type && chunk) {
 		if (chunk->end_of_packet || chunk->singleton)
-			error = sctp_outq_uncork(&asoc->outqueue, gfp);
+			sctp_outq_uncork(&asoc->outqueue, gfp);
 	} else if (local_cork)
-		error = sctp_outq_uncork(&asoc->outqueue, gfp);
+		sctp_outq_uncork(&asoc->outqueue, gfp);
 
 	if (sp->data_ready_signalled)
 		sp->data_ready_signalled = 0;
-- 
2.1.0

[PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Xin Long]

As David and Marcelo's suggestion, ENOMEM err shouldn't return back to
user in transmit path. Instead, sctp's retransmit would take care of
the chunks that fail to send because of ENOMEM.

This patch is only to do some release job when alloc_skb fails, not to
return ENOMEM back any more.

Besides, it also cleans up sctp_packet_transmit's err path, and fixes
some issues in err path:

 - It didn't free the head skb in nomem: path.
 - No need to check nskb in no_route: path.
 - It should goto err: path if alloc_skb fails for head.
 - Not all the NOMEMs should free nskb.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/output.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/net/sctp/output.c b/net/sctp/output.c
index 1934933..8f490ff 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -442,14 +442,14 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
 			 * time. Application may notice this error.
 			 */
 			pr_err_once("Trying to GSO but underlying device doesn't support it.");
-			goto nomem;
+			goto err;
 		}
 	} else {
 		pkt_size = packet->size;
 	}
 	head = alloc_skb(pkt_size + MAX_HEADER, gfp);
 	if (!head)
-		goto nomem;
+		goto err;
 	if (gso) {
 		NAPI_GRO_CB(head)->last = head;
 		skb_shinfo(head)->gso_type = sk->sk_gso_type;
@@ -470,8 +470,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
 		}
 	}
 	dst = dst_clone(tp->dst);
-	if (!dst)
-		goto no_route;
+	if (!dst) {
+		if (asoc)
+			IP_INC_STATS(sock_net(asoc->base.sk),
+				     IPSTATS_MIB_OUTNOROUTES);
+		goto nodst;
+	}
 	skb_dst_set(head, dst);
 
 	/* Build the SCTP header.  */
@@ -622,8 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
 		if (!gso)
 			break;
 
-		if (skb_gro_receive(&head, nskb))
+		if (skb_gro_receive(&head, nskb)) {
+			kfree_skb(nskb);
 			goto nomem;
+		}
 		nskb = NULL;
 		if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >=
 				 sk->sk_gso_max_segs))
@@ -717,18 +723,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
 	}
 	head->ignore_df = packet->ipfragok;
 	tp->af_specific->sctp_xmit(head, tp);
+	goto out;
 
-out:
-	sctp_packet_reset(packet);
-	return err;
-no_route:
-	kfree_skb(head);
-	if (nskb != head)
-		kfree_skb(nskb);
-
-	if (asoc)
-		IP_INC_STATS(sock_net(asoc->base.sk), IPSTATS_MIB_OUTNOROUTES);
+nomem:
+	if (packet->auth && list_empty(&packet->auth->list))
+		sctp_chunk_free(packet->auth);
 
+nodst:
 	/* FIXME: Returning the 'err' will effect all the associations
 	 * associated with a socket, although only one of the paths of the
 	 * association is unreachable.
@@ -737,22 +738,18 @@ no_route:
 	 * required.
 	 */
 	 /* err = -EHOSTUNREACH; */
-err:
-	/* Control chunks are unreliable so just drop them.  DATA chunks
-	 * will get resent or dropped later.
-	 */
+	kfree_skb(head);
 
+err:
 	list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) {
 		list_del_init(&chunk->list);
 		if (!sctp_chunk_is_data(chunk))
 			sctp_chunk_free(chunk);
 	}
-	goto out;
-nomem:
-	if (packet->auth && list_empty(&packet->auth->list))
-		sctp_chunk_free(packet->auth);
-	err = -ENOMEM;
-	goto err;
+
+out:
+	sctp_packet_reset(packet);
+	return err;
 }
 
 /********************************************************************
-- 
2.1.0