[PATCHv3 net-next 0/4] sctp: add sender-side procedures for stream reconf asoc reset and add streams

[PATCHv3 net-next 0/4] sctp: add sender-side procedures for stream reconf asoc reset and add streams

[Xin Long]

Patch 2/4 is to implement sender-side procedures for the SSN/TSN Reset
Request Parameter described in rfc6525 section 5.1.4, patch 1/4 is
ahead of it to define a function to make the request chunk for it.

Patch 4/4 is to implement sender-side procedures for the Add Incoming
and Outgoing Streams Request Parameter Request Parameter described in
rfc6525 section 5.1.5 and 5.1.6, patch 3/4 is ahead of it to define a
function to make the request chunk for it.

v1->v2:
  - put these into a smaller group.
  - rename some temporary variables in the codes.
  - rename the titles of the commits and improve some changelogs.
v2->v3:
  - re-split the patchset and make sure it has no dead codes for review.
  - move some codes into stream.c from socket.c.

Xin Long (4):
  sctp: add support for generating stream reconf ssn/tsn reset request
    chunk
  sctp: implement sender-side procedures for SSN/TSN Reset Request
    Parameter
  sctp: add support for generating stream reconf add incoming/outgoing
    streams request chunk
  sctp: implement sender-side procedures for Add Incoming/Outgoing
    Streams Request Parameter

 include/linux/sctp.h      |  12 +++++
 include/net/sctp/sctp.h   |   3 ++
 include/net/sctp/sm.h     |   5 ++
 include/uapi/linux/sctp.h |   8 +++
 net/sctp/sm_make_chunk.c  |  75 ++++++++++++++++++++++++++
 net/sctp/socket.c         |  58 ++++++++++++++++++++
 net/sctp/stream.c         | 132 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 293 insertions(+)

-- 
2.1.0

[PATCHv3 net-next 1/4] sctp: add support for generating stream reconf ssn/tsn reset request chunk

[Xin Long]

This patch is to define SSN/TSN Reset Request Parameter described
in rfc6525 section 4.3.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/sctp.h     |  5 +++++
 include/net/sctp/sm.h    |  2 ++
 net/sctp/sm_make_chunk.c | 29 +++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/include/linux/sctp.h b/include/linux/sctp.h
index a9e7906..95b8ed3 100644
--- a/include/linux/sctp.h
+++ b/include/linux/sctp.h
@@ -737,4 +737,9 @@ struct sctp_strreset_inreq {
 	__u16 list_of_streams[0];
 } __packed;
 
+struct sctp_strreset_tsnreq {
+	sctp_paramhdr_t param_hdr;
+	__u32 request_seq;
+} __packed;
+
 #endif /* __LINUX_SCTP_H__ */
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 430ed13..ac37c17 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -265,6 +265,8 @@ struct sctp_chunk *sctp_make_strreset_req(
 				const struct sctp_association *asoc,
 				__u16 stream_num, __u16 *stream_list,
 				bool out, bool in);
+struct sctp_chunk *sctp_make_strreset_tsnreq(
+				const struct sctp_association *asoc);
 void sctp_chunk_assign_tsn(struct sctp_chunk *);
 void sctp_chunk_assign_ssn(struct sctp_chunk *);
 
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index ad3445b..801450c 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3660,3 +3660,32 @@ struct sctp_chunk *sctp_make_strreset_req(
 
 	return retval;
 }
+
+/* RE-CONFIG 4.3 (SSN/TSN RESET ALL)
+ *   0                   1                   2                   3
+ *   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |     Parameter Type = 15       |      Parameter Length = 8     |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |         Re-configuration Request Sequence Number              |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct sctp_chunk *sctp_make_strreset_tsnreq(
+				const struct sctp_association *asoc)
+{
+	struct sctp_strreset_tsnreq tsnreq;
+	__u16 length = sizeof(tsnreq);
+	struct sctp_chunk *retval;
+
+	retval = sctp_make_reconf(asoc, length);
+	if (!retval)
+		return NULL;
+
+	tsnreq.param_hdr.type = SCTP_PARAM_RESET_TSN_REQUEST;
+	tsnreq.param_hdr.length = htons(length);
+	tsnreq.request_seq = htonl(asoc->strreset_outseq);
+
+	sctp_addto_chunk(retval, sizeof(tsnreq), &tsnreq);
+
+	return retval;
+}
-- 
2.1.0

[PATCHv3 net-next 2/4] sctp: implement sender-side procedures for SSN/TSN Reset Request Parameter

[Xin Long]

This patch is to implement Sender-Side Procedures for the SSN/TSN
Reset Request Parameter descibed in rfc6525 section 5.1.4.

It is also to add sockopt SCTP_RESET_ASSOC in rfc6525 section 6.3.3
for users.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/sctp.h   |  1 +
 include/uapi/linux/sctp.h |  1 +
 net/sctp/socket.c         | 29 +++++++++++++++++++++++++++++
 net/sctp/stream.c         | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 3cfd365b..b93820f 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -198,6 +198,7 @@ int sctp_offload_init(void);
  */
 int sctp_send_reset_streams(struct sctp_association *asoc,
 			    struct sctp_reset_streams *params);
+int sctp_send_reset_assoc(struct sctp_association *asoc);
 
 /*
  * Module global variables
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index 03c27ce..c0bd8c3 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -117,6 +117,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_PR_ASSOC_STATUS	115
 #define SCTP_ENABLE_STREAM_RESET	118
 #define SCTP_RESET_STREAMS	119
+#define SCTP_RESET_ASSOC	120
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index bee4dd3..2c5c9ca 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3812,6 +3812,32 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
 	return retval;
 }
 
+static int sctp_setsockopt_reset_assoc(struct sock *sk,
+				       char __user *optval,
+				       unsigned int optlen)
+{
+	struct sctp_association *asoc;
+	sctp_assoc_t associd;
+	int retval = -EINVAL;
+
+	if (optlen != sizeof(associd))
+		goto out;
+
+	if (copy_from_user(&associd, optval, optlen)) {
+		retval = -EFAULT;
+		goto out;
+	}
+
+	asoc = sctp_id2assoc(sk, associd);
+	if (!asoc)
+		goto out;
+
+	retval = sctp_send_reset_assoc(asoc);
+
+out:
+	return retval;
+}
+
 /* API 6.2 setsockopt(), getsockopt()
  *
  * Applications use setsockopt() and getsockopt() to set or retrieve
@@ -3984,6 +4010,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 	case SCTP_RESET_STREAMS:
 		retval = sctp_setsockopt_reset_streams(sk, optval, optlen);
 		break;
+	case SCTP_RESET_ASSOC:
+		retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 13d5e07..b368191 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -162,3 +162,36 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 out:
 	return retval;
 }
+
+int sctp_send_reset_assoc(struct sctp_association *asoc)
+{
+	struct sctp_chunk *chunk = NULL;
+	int retval;
+	__u16 i;
+
+	if (!asoc->peer.reconf_capable ||
+	    !(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
+		return -ENOPROTOOPT;
+
+	if (asoc->strreset_outstanding)
+		return -EINPROGRESS;
+
+	chunk = sctp_make_strreset_tsnreq(asoc);
+	if (!chunk)
+		return -ENOMEM;
+
+	for (i = 0; i < asoc->stream->outcnt; i++)
+		asoc->stream->out[i].state = SCTP_STREAM_CLOSED;
+
+	asoc->strreset_outstanding = 1;
+	asoc->strreset_chunk = chunk;
+	sctp_chunk_hold(asoc->strreset_chunk);
+
+	retval = sctp_send_reconf(asoc, chunk);
+	if (retval) {
+		sctp_chunk_put(asoc->strreset_chunk);
+		asoc->strreset_chunk = NULL;
+	}
+
+	return retval;
+}
-- 
2.1.0

[PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing streams

[Xin Long]

This patch is to define Add Incoming/Outgoing Streams Request
Parameter described in rfc6525 section 4.5 and 4.6. They can
be in one same chunk trunk as rfc6525 section 3.1-7 describes,
so make them in one function.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/sctp.h     |  7 +++++++
 include/net/sctp/sm.h    |  3 +++
 net/sctp/sm_make_chunk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/include/linux/sctp.h b/include/linux/sctp.h
index 95b8ed3..f1f494f 100644
--- a/include/linux/sctp.h
+++ b/include/linux/sctp.h
@@ -742,4 +742,11 @@ struct sctp_strreset_tsnreq {
 	__u32 request_seq;
 } __packed;
 
+struct sctp_strreset_addstrm {
+	sctp_paramhdr_t param_hdr;
+	__u32 request_seq;
+	__u16 number_of_streams;
+	__u16 reserved;
+} __packed;
+
 #endif /* __LINUX_SCTP_H__ */
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index ac37c17..3675fde 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -267,6 +267,9 @@ struct sctp_chunk *sctp_make_strreset_req(
 				bool out, bool in);
 struct sctp_chunk *sctp_make_strreset_tsnreq(
 				const struct sctp_association *asoc);
+struct sctp_chunk *sctp_make_strreset_addstrm(
+				const struct sctp_association *asoc,
+				__u16 out, __u16 in);
 void sctp_chunk_assign_tsn(struct sctp_chunk *);
 void sctp_chunk_assign_ssn(struct sctp_chunk *);
 
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 801450c..a44546d 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3689,3 +3689,49 @@ struct sctp_chunk *sctp_make_strreset_tsnreq(
 
 	return retval;
 }
+
+/* RE-CONFIG 4.5/4.6 (ADD STREAM)
+ *   0                   1                   2                   3
+ *   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |     Parameter Type = 17       |      Parameter Length = 12    |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |          Re-configuration Request Sequence Number             |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |      Number of new streams    |         Reserved              |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+struct sctp_chunk *sctp_make_strreset_addstrm(
+				const struct sctp_association *asoc,
+				__u16 out, __u16 in)
+{
+	struct sctp_strreset_addstrm addstrm;
+	__u16 size = sizeof(addstrm);
+	struct sctp_chunk *retval;
+
+	retval = sctp_make_reconf(asoc, (!!out + !!in) * size);
+	if (!retval)
+		return NULL;
+
+	if (out) {
+		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
+		addstrm.param_hdr.length = htons(size);
+		addstrm.number_of_streams = htons(out);
+		addstrm.request_seq = htonl(asoc->strreset_outseq);
+		addstrm.reserved = 0;
+
+		sctp_addto_chunk(retval, size, &addstrm);
+	}
+
+	if (in) {
+		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_IN_STREAMS;
+		addstrm.param_hdr.length = htons(size);
+		addstrm.number_of_streams = htons(in);
+		addstrm.request_seq = htonl(asoc->strreset_outseq + !!out);
+		addstrm.reserved = 0;
+
+		sctp_addto_chunk(retval, size, &addstrm);
+	}
+
+	return retval;
+}
-- 
2.1.0

[PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Requ

[Xin Long]

This patch is to implement Sender-Side Procedures for the Add
Outgoing and Incoming Streams Request Parameter described in
rfc6525 section 5.1.5-5.1.6.

It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
6.3.4 for users.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/sctp.h   |  2 +
 include/uapi/linux/sctp.h |  7 ++++
 net/sctp/socket.c         | 29 ++++++++++++++
 net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 137 insertions(+)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index b93820f..68ee1a6 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -199,6 +199,8 @@ int sctp_offload_init(void);
 int sctp_send_reset_streams(struct sctp_association *asoc,
 			    struct sctp_reset_streams *params);
 int sctp_send_reset_assoc(struct sctp_association *asoc);
+int sctp_send_add_streams(struct sctp_association *asoc,
+			  struct sctp_add_streams *params);
 
 /*
  * Module global variables
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index c0bd8c3..a91a9cc 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_ENABLE_STREAM_RESET	118
 #define SCTP_RESET_STREAMS	119
 #define SCTP_RESET_ASSOC	120
+#define SCTP_ADD_STREAMS	121
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
@@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
 	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
 };
 
+struct sctp_add_streams {
+	sctp_assoc_t sas_assoc_id;
+	uint16_t sas_instrms;
+	uint16_t sas_outstrms;
+};
+
 #endif /* _UAPI_SCTP_H */
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 2c5c9ca..ae0a99e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
 	return retval;
 }
 
+static int sctp_setsockopt_add_streams(struct sock *sk,
+				       char __user *optval,
+				       unsigned int optlen)
+{
+	struct sctp_association *asoc;
+	struct sctp_add_streams params;
+	int retval = -EINVAL;
+
+	if (optlen != sizeof(params))
+		goto out;
+
+	if (copy_from_user(&params, optval, optlen)) {
+		retval = -EFAULT;
+		goto out;
+	}
+
+	asoc = sctp_id2assoc(sk, params.sas_assoc_id);
+	if (!asoc)
+		goto out;
+
+	retval = sctp_send_add_streams(asoc, &params);
+
+out:
+	return retval;
+}
+
 /* API 6.2 setsockopt(), getsockopt()
  *
  * Applications use setsockopt() and getsockopt() to set or retrieve
@@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 	case SCTP_RESET_ASSOC:
 		retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
 		break;
+	case SCTP_ADD_STREAMS:
+		retval = sctp_setsockopt_add_streams(sk, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index b368191..ba41837 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
 
 	return retval;
 }
+
+int sctp_send_add_streams(struct sctp_association *asoc,
+			  struct sctp_add_streams *params)
+{
+	struct sctp_stream *stream = asoc->stream;
+	struct sctp_chunk *chunk = NULL;
+	int retval = -EINVAL;
+	__u16 out, in;
+
+	if (!asoc->peer.reconf_capable ||
+	    !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
+		retval = -ENOPROTOOPT;
+		goto out;
+	}
+
+	if (asoc->strreset_outstanding) {
+		retval = -EINPROGRESS;
+		goto out;
+	}
+
+	out = params->sas_outstrms;
+	in  = params->sas_instrms;
+
+	if (!out && !in)
+		goto out;
+
+	if (out) {
+		__u16 nums = stream->outcnt + out;
+
+		/* Check for overflow, can't use nums here */
+		if (stream->outcnt + out > SCTP_MAX_STREAM)
+			goto out;
+
+		/* Use ksize to check if stream array really needs to realloc */
+		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
+			struct sctp_stream_out *streamout;
+
+			streamout = kcalloc(nums, sizeof(*streamout),
+					    GFP_KERNEL);
+			if (!streamout) {
+				retval = -ENOMEM;
+				goto out;
+			}
+
+			memcpy(streamout, stream->out,
+			       sizeof(*streamout) * stream->outcnt);
+
+			kfree(stream->out);
+			stream->out = streamout;
+		}
+
+		stream->outcnt = nums;
+	}
+
+	if (in) {
+		__u16 nums = stream->incnt + in;
+
+		if (stream->incnt + in > SCTP_MAX_STREAM)
+			goto out;
+
+		if (ksize(stream->in) / sizeof(*stream->in) < nums) {
+			struct sctp_stream_in *streamin;
+
+			streamin = kcalloc(nums, sizeof(*streamin),
+					   GFP_KERNEL);
+			if (!streamin) {
+				retval = -ENOMEM;
+				goto out;
+			}
+
+			memcpy(streamin, stream->in,
+			       sizeof(*streamin) * stream->incnt);
+
+			kfree(stream->in);
+			stream->in = streamin;
+		}
+
+		stream->incnt = nums;
+	}
+
+	chunk = sctp_make_strreset_addstrm(asoc, out, in);
+	if (!chunk) {
+		retval = -ENOMEM;
+		goto out;
+	}
+
+	asoc->strreset_outstanding = !!out + !!in;
+	asoc->strreset_chunk = chunk;
+	sctp_chunk_hold(asoc->strreset_chunk);
+
+	retval = sctp_send_reconf(asoc, chunk);
+	if (retval) {
+		sctp_chunk_put(asoc->strreset_chunk);
+		asoc->strreset_chunk = NULL;
+	}
+
+out:
+	return retval;
+}
-- 
2.1.0

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Neil Horman]

On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> This patch is to implement Sender-Side Procedures for the Add
> Outgoing and Incoming Streams Request Parameter described in
> rfc6525 section 5.1.5-5.1.6.
> 
> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> 6.3.4 for users.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/sctp.h   |  2 +
>  include/uapi/linux/sctp.h |  7 ++++
>  net/sctp/socket.c         | 29 ++++++++++++++
>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index b93820f..68ee1a6 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params);
>  int sctp_send_reset_assoc(struct sctp_association *asoc);
> +int sctp_send_add_streams(struct sctp_association *asoc,
> +			  struct sctp_add_streams *params);
>  
>  /*
>   * Module global variables
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index c0bd8c3..a91a9cc 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_ENABLE_STREAM_RESET	118
>  #define SCTP_RESET_STREAMS	119
>  #define SCTP_RESET_ASSOC	120
> +#define SCTP_ADD_STREAMS	121
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
>  	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
>  };
>  
> +struct sctp_add_streams {
> +	sctp_assoc_t sas_assoc_id;
> +	uint16_t sas_instrms;
> +	uint16_t sas_outstrms;
> +};
> +
>  #endif /* _UAPI_SCTP_H */
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 2c5c9ca..ae0a99e 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
>  	return retval;
>  }
>  
> +static int sctp_setsockopt_add_streams(struct sock *sk,
> +				       char __user *optval,
> +				       unsigned int optlen)
> +{
you are going to need to provide some locking here or in sctp_send_add_streams.
By replacing the in/out streams pointer you run the risk of multiple callers
modifying the pointers in parallel, or in the sctp state machine reading it
while you do.

Neil

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Thu, Jan 19, 2017 at 03:17:18PM -0500, Neil Horman wrote:
> On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> > This patch is to implement Sender-Side Procedures for the Add
> > Outgoing and Incoming Streams Request Parameter described in
> > rfc6525 section 5.1.5-5.1.6.
> > 
> > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > 6.3.4 for users.
> > 
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/sctp/sctp.h   |  2 +
> >  include/uapi/linux/sctp.h |  7 ++++
> >  net/sctp/socket.c         | 29 ++++++++++++++
> >  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 137 insertions(+)
> > 
> > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> > index b93820f..68ee1a6 100644
> > --- a/include/net/sctp/sctp.h
> > +++ b/include/net/sctp/sctp.h
> > @@ -199,6 +199,8 @@ int sctp_offload_init(void);
> >  int sctp_send_reset_streams(struct sctp_association *asoc,
> >  			    struct sctp_reset_streams *params);
> >  int sctp_send_reset_assoc(struct sctp_association *asoc);
> > +int sctp_send_add_streams(struct sctp_association *asoc,
> > +			  struct sctp_add_streams *params);
> >  
> >  /*
> >   * Module global variables
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index c0bd8c3..a91a9cc 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
> >  #define SCTP_ENABLE_STREAM_RESET	118
> >  #define SCTP_RESET_STREAMS	119
> >  #define SCTP_RESET_ASSOC	120
> > +#define SCTP_ADD_STREAMS	121
> >  
> >  /* PR-SCTP policies */
> >  #define SCTP_PR_SCTP_NONE	0x0000
> > @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
> >  	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
> >  };
> >  
> > +struct sctp_add_streams {
> > +	sctp_assoc_t sas_assoc_id;
> > +	uint16_t sas_instrms;
> > +	uint16_t sas_outstrms;
> > +};
> > +
> >  #endif /* _UAPI_SCTP_H */
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 2c5c9ca..ae0a99e 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
> >  	return retval;
> >  }
> >  
> > +static int sctp_setsockopt_add_streams(struct sock *sk,
> > +				       char __user *optval,
> > +				       unsigned int optlen)
> > +{
> you are going to need to provide some locking here or in sctp_send_add_streams.
> By replacing the in/out streams pointer you run the risk of multiple callers
> modifying the pointers in parallel, or in the sctp state machine reading it
> while you do.

There can't be multiple callers here because the socket is locked before
calling this function, in sctp_setsockopt().
I also don't see how a BH handler, timers or sleeping call (recvmsg)
could trip on this.

  Marcelo

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Neil Horman]

On Thu, Jan 19, 2017 at 08:18:13PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Jan 19, 2017 at 03:17:18PM -0500, Neil Horman wrote:
> > On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> > > This patch is to implement Sender-Side Procedures for the Add
> > > Outgoing and Incoming Streams Request Parameter described in
> > > rfc6525 section 5.1.5-5.1.6.
> > > 
> > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > 6.3.4 for users.
> > > 
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  include/net/sctp/sctp.h   |  2 +
> > >  include/uapi/linux/sctp.h |  7 ++++
> > >  net/sctp/socket.c         | 29 ++++++++++++++
> > >  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 137 insertions(+)
> > > 
> > > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> > > index b93820f..68ee1a6 100644
> > > --- a/include/net/sctp/sctp.h
> > > +++ b/include/net/sctp/sctp.h
> > > @@ -199,6 +199,8 @@ int sctp_offload_init(void);
> > >  int sctp_send_reset_streams(struct sctp_association *asoc,
> > >  			    struct sctp_reset_streams *params);
> > >  int sctp_send_reset_assoc(struct sctp_association *asoc);
> > > +int sctp_send_add_streams(struct sctp_association *asoc,
> > > +			  struct sctp_add_streams *params);
> > >  
> > >  /*
> > >   * Module global variables
> > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > > index c0bd8c3..a91a9cc 100644
> > > --- a/include/uapi/linux/sctp.h
> > > +++ b/include/uapi/linux/sctp.h
> > > @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
> > >  #define SCTP_ENABLE_STREAM_RESET	118
> > >  #define SCTP_RESET_STREAMS	119
> > >  #define SCTP_RESET_ASSOC	120
> > > +#define SCTP_ADD_STREAMS	121
> > >  
> > >  /* PR-SCTP policies */
> > >  #define SCTP_PR_SCTP_NONE	0x0000
> > > @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
> > >  	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
> > >  };
> > >  
> > > +struct sctp_add_streams {
> > > +	sctp_assoc_t sas_assoc_id;
> > > +	uint16_t sas_instrms;
> > > +	uint16_t sas_outstrms;
> > > +};
> > > +
> > >  #endif /* _UAPI_SCTP_H */
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index 2c5c9ca..ae0a99e 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
> > >  	return retval;
> > >  }
> > >  
> > > +static int sctp_setsockopt_add_streams(struct sock *sk,
> > > +				       char __user *optval,
> > > +				       unsigned int optlen)
> > > +{
> > you are going to need to provide some locking here or in sctp_send_add_streams.
> > By replacing the in/out streams pointer you run the risk of multiple callers
> > modifying the pointers in parallel, or in the sctp state machine reading it
> > while you do.
> 
> There can't be multiple callers here because the socket is locked before
> calling this function, in sctp_setsockopt().
> I also don't see how a BH handler, timers or sleeping call (recvmsg)
> could trip on this.
You're right, I missed the socket lock in sctp_setsockopt, that covers us both
from multiple processes and bottom halves.

> 
>   Marcelo
> 
> --
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> This patch is to implement Sender-Side Procedures for the Add
> Outgoing and Incoming Streams Request Parameter described in
> rfc6525 section 5.1.5-5.1.6.
> 
> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> 6.3.4 for users.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/sctp.h   |  2 +
>  include/uapi/linux/sctp.h |  7 ++++
>  net/sctp/socket.c         | 29 ++++++++++++++
>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index b93820f..68ee1a6 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params);
>  int sctp_send_reset_assoc(struct sctp_association *asoc);
> +int sctp_send_add_streams(struct sctp_association *asoc,
> +			  struct sctp_add_streams *params);
>  
>  /*
>   * Module global variables
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index c0bd8c3..a91a9cc 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_ENABLE_STREAM_RESET	118
>  #define SCTP_RESET_STREAMS	119
>  #define SCTP_RESET_ASSOC	120
> +#define SCTP_ADD_STREAMS	121
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
>  	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
>  };
>  
> +struct sctp_add_streams {
> +	sctp_assoc_t sas_assoc_id;
> +	uint16_t sas_instrms;
> +	uint16_t sas_outstrms;
> +};
> +
>  #endif /* _UAPI_SCTP_H */
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 2c5c9ca..ae0a99e 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
>  	return retval;
>  }
>  
> +static int sctp_setsockopt_add_streams(struct sock *sk,
> +				       char __user *optval,
> +				       unsigned int optlen)
> +{
> +	struct sctp_association *asoc;
> +	struct sctp_add_streams params;
> +	int retval = -EINVAL;
> +
> +	if (optlen != sizeof(params))
> +		goto out;
> +
> +	if (copy_from_user(&params, optval, optlen)) {
> +		retval = -EFAULT;
> +		goto out;
> +	}
> +
> +	asoc = sctp_id2assoc(sk, params.sas_assoc_id);
> +	if (!asoc)
> +		goto out;
> +
> +	retval = sctp_send_add_streams(asoc, &params);
> +
> +out:
> +	return retval;
> +}
> +
>  /* API 6.2 setsockopt(), getsockopt()
>   *
>   * Applications use setsockopt() and getsockopt() to set or retrieve
> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>  	case SCTP_RESET_ASSOC:
>  		retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
>  		break;
> +	case SCTP_ADD_STREAMS:
> +		retval = sctp_setsockopt_add_streams(sk, optval, optlen);
> +		break;
>  	default:
>  		retval = -ENOPROTOOPT;
>  		break;
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index b368191..ba41837 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>  
>  	return retval;
>  }
> +
> +int sctp_send_add_streams(struct sctp_association *asoc,
> +			  struct sctp_add_streams *params)
> +{
> +	struct sctp_stream *stream = asoc->stream;
> +	struct sctp_chunk *chunk = NULL;
> +	int retval = -EINVAL;
> +	__u16 out, in;
> +
> +	if (!asoc->peer.reconf_capable ||
> +	    !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
> +		retval = -ENOPROTOOPT;
> +		goto out;
> +	}
> +
> +	if (asoc->strreset_outstanding) {
> +		retval = -EINPROGRESS;
> +		goto out;
> +	}
> +
> +	out = params->sas_outstrms;
> +	in  = params->sas_instrms;
> +
> +	if (!out && !in)

[@]

> +		goto out;
> +
> +	if (out) {
> +		__u16 nums = stream->outcnt + out;
> +
> +		/* Check for overflow, can't use nums here */
> +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> +			goto out;

You should do these overflow checks before doing actual work,
preferreably at [@] mark above.
Here it's fine, but when [*] below run, it may be too late.

> +
> +		/* Use ksize to check if stream array really needs to realloc */
> +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> +			struct sctp_stream_out *streamout;
> +
> +			streamout = kcalloc(nums, sizeof(*streamout),
> +					    GFP_KERNEL);
> +			if (!streamout) {
> +				retval = -ENOMEM;
> +				goto out;
> +			}
> +
> +			memcpy(streamout, stream->out,
> +			       sizeof(*streamout) * stream->outcnt);
> +
> +			kfree(stream->out);
> +			stream->out = streamout;
> +		}
> +
> +		stream->outcnt = nums;
> +	}
> +
> +	if (in) {
> +		__u16 nums = stream->incnt + in;
> +
> +		if (stream->incnt + in > SCTP_MAX_STREAM)
> +			goto out;

[*]

> +
> +		if (ksize(stream->in) / sizeof(*stream->in) < nums) {
> +			struct sctp_stream_in *streamin;
> +
> +			streamin = kcalloc(nums, sizeof(*streamin),
> +					   GFP_KERNEL);
> +			if (!streamin) {
> +				retval = -ENOMEM;
> +				goto out;
> +			}
> +
> +			memcpy(streamin, stream->in,
> +			       sizeof(*streamin) * stream->incnt);
> +
> +			kfree(stream->in);
> +			stream->in = streamin;
> +		}
> +
> +		stream->incnt = nums;
> +	}
> +
> +	chunk = sctp_make_strreset_addstrm(asoc, out, in);
> +	if (!chunk) {
> +		retval = -ENOMEM;
> +		goto out;
> +	}
> +
> +	asoc->strreset_outstanding = !!out + !!in;
> +	asoc->strreset_chunk = chunk;
> +	sctp_chunk_hold(asoc->strreset_chunk);
> +
> +	retval = sctp_send_reconf(asoc, chunk);
> +	if (retval) {
> +		sctp_chunk_put(asoc->strreset_chunk);
> +		asoc->strreset_chunk = NULL;
> +	}
> +
> +out:
> +	return retval;
> +}
> -- 
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Xin Long]

On Fri, Jan 20, 2017 at 5:47 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
>> This patch is to implement Sender-Side Procedures for the Add
>> Outgoing and Incoming Streams Request Parameter described in
>> rfc6525 section 5.1.5-5.1.6.
>>
>> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
>> 6.3.4 for users.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>>  include/net/sctp/sctp.h   |  2 +
>>  include/uapi/linux/sctp.h |  7 ++++
>>  net/sctp/socket.c         | 29 ++++++++++++++
>>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index b93820f..68ee1a6 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
>>  int sctp_send_reset_streams(struct sctp_association *asoc,
>>                           struct sctp_reset_streams *params);
>>  int sctp_send_reset_assoc(struct sctp_association *asoc);
>> +int sctp_send_add_streams(struct sctp_association *asoc,
>> +                       struct sctp_add_streams *params);
>>
>>  /*
>>   * Module global variables
>> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>> index c0bd8c3..a91a9cc 100644
>> --- a/include/uapi/linux/sctp.h
>> +++ b/include/uapi/linux/sctp.h
>> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
>>  #define SCTP_ENABLE_STREAM_RESET     118
>>  #define SCTP_RESET_STREAMS   119
>>  #define SCTP_RESET_ASSOC     120
>> +#define SCTP_ADD_STREAMS     121
>>
>>  /* PR-SCTP policies */
>>  #define SCTP_PR_SCTP_NONE    0x0000
>> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
>>       uint16_t srs_stream_list[];     /* list if srs_num_streams is not 0 */
>>  };
>>
>> +struct sctp_add_streams {
>> +     sctp_assoc_t sas_assoc_id;
>> +     uint16_t sas_instrms;
>> +     uint16_t sas_outstrms;
>> +};
>> +
>>  #endif /* _UAPI_SCTP_H */
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 2c5c9ca..ae0a99e 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
>>       return retval;
>>  }
>>
>> +static int sctp_setsockopt_add_streams(struct sock *sk,
>> +                                    char __user *optval,
>> +                                    unsigned int optlen)
>> +{
>> +     struct sctp_association *asoc;
>> +     struct sctp_add_streams params;
>> +     int retval = -EINVAL;
>> +
>> +     if (optlen != sizeof(params))
>> +             goto out;
>> +
>> +     if (copy_from_user(&params, optval, optlen)) {
>> +             retval = -EFAULT;
>> +             goto out;
>> +     }
>> +
>> +     asoc = sctp_id2assoc(sk, params.sas_assoc_id);
>> +     if (!asoc)
>> +             goto out;
>> +
>> +     retval = sctp_send_add_streams(asoc, &params);
>> +
>> +out:
>> +     return retval;
>> +}
>> +
>>  /* API 6.2 setsockopt(), getsockopt()
>>   *
>>   * Applications use setsockopt() and getsockopt() to set or retrieve
>> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>>       case SCTP_RESET_ASSOC:
>>               retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
>>               break;
>> +     case SCTP_ADD_STREAMS:
>> +             retval = sctp_setsockopt_add_streams(sk, optval, optlen);
>> +             break;
>>       default:
>>               retval = -ENOPROTOOPT;
>>               break;
>> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> index b368191..ba41837 100644
>> --- a/net/sctp/stream.c
>> +++ b/net/sctp/stream.c
>> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>>
>>       return retval;
>>  }
>> +
>> +int sctp_send_add_streams(struct sctp_association *asoc,
>> +                       struct sctp_add_streams *params)
>> +{
>> +     struct sctp_stream *stream = asoc->stream;
>> +     struct sctp_chunk *chunk = NULL;
>> +     int retval = -EINVAL;
>> +     __u16 out, in;
>> +
>> +     if (!asoc->peer.reconf_capable ||
>> +         !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
>> +             retval = -ENOPROTOOPT;
>> +             goto out;
>> +     }
>> +
>> +     if (asoc->strreset_outstanding) {
>> +             retval = -EINPROGRESS;
>> +             goto out;
>> +     }
>> +
>> +     out = params->sas_outstrms;
>> +     in  = params->sas_instrms;
>> +
>> +     if (!out && !in)
>
> [@]
>
>> +             goto out;
>> +
>> +     if (out) {
>> +             __u16 nums = stream->outcnt + out;
>> +
>> +             /* Check for overflow, can't use nums here */
>> +             if (stream->outcnt + out > SCTP_MAX_STREAM)
>> +                     goto out;
>
> You should do these overflow checks before doing actual work,
> preferreably at [@] mark above.
> Here it's fine, but when [*] below run, it may be too late.
>
>> +
>> +             /* Use ksize to check if stream array really needs to realloc */
>> +             if (ksize(stream->out) / sizeof(*stream->out) < nums) {
>> +                     struct sctp_stream_out *streamout;
>> +
>> +                     streamout = kcalloc(nums, sizeof(*streamout),
>> +                                         GFP_KERNEL);
>> +                     if (!streamout) {
>> +                             retval = -ENOMEM;
>> +                             goto out;
>> +                     }
>> +
>> +                     memcpy(streamout, stream->out,
>> +                            sizeof(*streamout) * stream->outcnt);
>> +
>> +                     kfree(stream->out);
>> +                     stream->out = streamout;
>> +             }
>> +
>> +             stream->outcnt = nums;
>> +     }
>> +
>> +     if (in) {
>> +             __u16 nums = stream->incnt + in;
>> +
>> +             if (stream->incnt + in > SCTP_MAX_STREAM)
>> +                     goto out;
>
> [*]
it will cause alloc unused memory, but no consistent issue if
we move outcnt/incnt after checking retval.

if we want overflow check to go ahead of the actual work, another
conditions if (in) {} if(out) {} have to be added.

>
>> +
>> +             if (ksize(stream->in) / sizeof(*stream->in) < nums) {
>> +                     struct sctp_stream_in *streamin;
>> +
>> +                     streamin = kcalloc(nums, sizeof(*streamin),
>> +                                        GFP_KERNEL);
>> +                     if (!streamin) {
>> +                             retval = -ENOMEM;
>> +                             goto out;
>> +                     }
>> +
>> +                     memcpy(streamin, stream->in,
>> +                            sizeof(*streamin) * stream->incnt);
>> +
>> +                     kfree(stream->in);
>> +                     stream->in = streamin;
>> +             }
>> +
>> +             stream->incnt = nums;
>> +     }
>> +
>> +     chunk = sctp_make_strreset_addstrm(asoc, out, in);
>> +     if (!chunk) {
>> +             retval = -ENOMEM;
>> +             goto out;
>> +     }
>> +
>> +     asoc->strreset_outstanding = !!out + !!in;
>> +     asoc->strreset_chunk = chunk;
>> +     sctp_chunk_hold(asoc->strreset_chunk);
>> +
>> +     retval = sctp_send_reconf(asoc, chunk);
>> +     if (retval) {
>> +             sctp_chunk_put(asoc->strreset_chunk);
>> +             asoc->strreset_chunk = NULL;
>> +     }
>> +
>> +out:
>> +     return retval;
>> +}
>> --
>> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 04:56:30PM +0800, Xin Long wrote:
> On Fri, Jan 20, 2017 at 5:47 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> >> This patch is to implement Sender-Side Procedures for the Add
> >> Outgoing and Incoming Streams Request Parameter described in
> >> rfc6525 section 5.1.5-5.1.6.
> >>
> >> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> >> 6.3.4 for users.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >> ---
> >>  include/net/sctp/sctp.h   |  2 +
> >>  include/uapi/linux/sctp.h |  7 ++++
> >>  net/sctp/socket.c         | 29 ++++++++++++++
> >>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 137 insertions(+)
> >>
> >> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >> index b93820f..68ee1a6 100644
> >> --- a/include/net/sctp/sctp.h
> >> +++ b/include/net/sctp/sctp.h
> >> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
> >>  int sctp_send_reset_streams(struct sctp_association *asoc,
> >>                           struct sctp_reset_streams *params);
> >>  int sctp_send_reset_assoc(struct sctp_association *asoc);
> >> +int sctp_send_add_streams(struct sctp_association *asoc,
> >> +                       struct sctp_add_streams *params);
> >>
> >>  /*
> >>   * Module global variables
> >> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >> index c0bd8c3..a91a9cc 100644
> >> --- a/include/uapi/linux/sctp.h
> >> +++ b/include/uapi/linux/sctp.h
> >> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
> >>  #define SCTP_ENABLE_STREAM_RESET     118
> >>  #define SCTP_RESET_STREAMS   119
> >>  #define SCTP_RESET_ASSOC     120
> >> +#define SCTP_ADD_STREAMS     121
> >>
> >>  /* PR-SCTP policies */
> >>  #define SCTP_PR_SCTP_NONE    0x0000
> >> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
> >>       uint16_t srs_stream_list[];     /* list if srs_num_streams is not 0 */
> >>  };
> >>
> >> +struct sctp_add_streams {
> >> +     sctp_assoc_t sas_assoc_id;
> >> +     uint16_t sas_instrms;
> >> +     uint16_t sas_outstrms;
> >> +};
> >> +
> >>  #endif /* _UAPI_SCTP_H */
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 2c5c9ca..ae0a99e 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
> >>       return retval;
> >>  }
> >>
> >> +static int sctp_setsockopt_add_streams(struct sock *sk,
> >> +                                    char __user *optval,
> >> +                                    unsigned int optlen)
> >> +{
> >> +     struct sctp_association *asoc;
> >> +     struct sctp_add_streams params;
> >> +     int retval = -EINVAL;
> >> +
> >> +     if (optlen != sizeof(params))
> >> +             goto out;
> >> +
> >> +     if (copy_from_user(&params, optval, optlen)) {
> >> +             retval = -EFAULT;
> >> +             goto out;
> >> +     }
> >> +
> >> +     asoc = sctp_id2assoc(sk, params.sas_assoc_id);
> >> +     if (!asoc)
> >> +             goto out;
> >> +
> >> +     retval = sctp_send_add_streams(asoc, &params);
> >> +
> >> +out:
> >> +     return retval;
> >> +}
> >> +
> >>  /* API 6.2 setsockopt(), getsockopt()
> >>   *
> >>   * Applications use setsockopt() and getsockopt() to set or retrieve
> >> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
> >>       case SCTP_RESET_ASSOC:
> >>               retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
> >>               break;
> >> +     case SCTP_ADD_STREAMS:
> >> +             retval = sctp_setsockopt_add_streams(sk, optval, optlen);
> >> +             break;
> >>       default:
> >>               retval = -ENOPROTOOPT;
> >>               break;
> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> >> index b368191..ba41837 100644
> >> --- a/net/sctp/stream.c
> >> +++ b/net/sctp/stream.c
> >> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
> >>
> >>       return retval;
> >>  }
> >> +
> >> +int sctp_send_add_streams(struct sctp_association *asoc,
> >> +                       struct sctp_add_streams *params)
> >> +{
> >> +     struct sctp_stream *stream = asoc->stream;
> >> +     struct sctp_chunk *chunk = NULL;
> >> +     int retval = -EINVAL;
> >> +     __u16 out, in;
> >> +
> >> +     if (!asoc->peer.reconf_capable ||
> >> +         !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
> >> +             retval = -ENOPROTOOPT;
> >> +             goto out;
> >> +     }
> >> +
> >> +     if (asoc->strreset_outstanding) {
> >> +             retval = -EINPROGRESS;
> >> +             goto out;
> >> +     }
> >> +
> >> +     out = params->sas_outstrms;
> >> +     in  = params->sas_instrms;
> >> +
> >> +     if (!out && !in)
> >
> > [@]
> >
> >> +             goto out;
> >> +
> >> +     if (out) {
> >> +             __u16 nums = stream->outcnt + out;
> >> +
> >> +             /* Check for overflow, can't use nums here */
> >> +             if (stream->outcnt + out > SCTP_MAX_STREAM)
> >> +                     goto out;
> >
> > You should do these overflow checks before doing actual work,
> > preferreably at [@] mark above.
> > Here it's fine, but when [*] below run, it may be too late.
> >
> >> +
> >> +             /* Use ksize to check if stream array really needs to realloc */
> >> +             if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> >> +                     struct sctp_stream_out *streamout;
> >> +
> >> +                     streamout = kcalloc(nums, sizeof(*streamout),
> >> +                                         GFP_KERNEL);
> >> +                     if (!streamout) {
> >> +                             retval = -ENOMEM;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     memcpy(streamout, stream->out,
> >> +                            sizeof(*streamout) * stream->outcnt);
> >> +
> >> +                     kfree(stream->out);
> >> +                     stream->out = streamout;
> >> +             }
> >> +
> >> +             stream->outcnt = nums;
> >> +     }
> >> +
> >> +     if (in) {
> >> +             __u16 nums = stream->incnt + in;
> >> +
> >> +             if (stream->incnt + in > SCTP_MAX_STREAM)
> >> +                     goto out;
> >
> > [*]
> it will cause alloc unused memory, but no consistent issue if
> we move outcnt/incnt after checking retval.

Ah yes, right, so it doesn't hurt actually.

> 
> if we want overflow check to go ahead of the actual work, another
> conditions if (in) {} if(out) {} have to be added.

Not sure why? Seems 
if (!in || !out || stream->incnt + in > SCTP_MAX_STREAM ||
    stream->outcnt + out > SCTP_MAX_STREAM)
	goto out;
would do it, no?

> 
> >
> >> +
> >> +             if (ksize(stream->in) / sizeof(*stream->in) < nums) {
> >> +                     struct sctp_stream_in *streamin;
> >> +
> >> +                     streamin = kcalloc(nums, sizeof(*streamin),
> >> +                                        GFP_KERNEL);
> >> +                     if (!streamin) {
> >> +                             retval = -ENOMEM;
> >> +                             goto out;
> >> +                     }
> >> +
> >> +                     memcpy(streamin, stream->in,
> >> +                            sizeof(*streamin) * stream->incnt);
> >> +
> >> +                     kfree(stream->in);
> >> +                     stream->in = streamin;
> >> +             }
> >> +
> >> +             stream->incnt = nums;
> >> +     }
> >> +
> >> +     chunk = sctp_make_strreset_addstrm(asoc, out, in);
> >> +     if (!chunk) {
> >> +             retval = -ENOMEM;
> >> +             goto out;
> >> +     }
> >> +
> >> +     asoc->strreset_outstanding = !!out + !!in;
> >> +     asoc->strreset_chunk = chunk;
> >> +     sctp_chunk_hold(asoc->strreset_chunk);
> >> +
> >> +     retval = sctp_send_reconf(asoc, chunk);
> >> +     if (retval) {
> >> +             sctp_chunk_put(asoc->strreset_chunk);
> >> +             asoc->strreset_chunk = NULL;
> >> +     }
> >> +
> >> +out:
> >> +     return retval;
> >> +}
> >> --
> >> 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
> >>
> --
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
> This patch is to implement Sender-Side Procedures for the Add
> Outgoing and Incoming Streams Request Parameter described in
> rfc6525 section 5.1.5-5.1.6.
> 
> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> 6.3.4 for users.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/sctp.h   |  2 +
>  include/uapi/linux/sctp.h |  7 ++++
>  net/sctp/socket.c         | 29 ++++++++++++++
>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index b93820f..68ee1a6 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params);
>  int sctp_send_reset_assoc(struct sctp_association *asoc);
> +int sctp_send_add_streams(struct sctp_association *asoc,
> +			  struct sctp_add_streams *params);
>  
>  /*
>   * Module global variables
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index c0bd8c3..a91a9cc 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_ENABLE_STREAM_RESET	118
>  #define SCTP_RESET_STREAMS	119
>  #define SCTP_RESET_ASSOC	120
> +#define SCTP_ADD_STREAMS	121
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
>  	uint16_t srs_stream_list[];	/* list if srs_num_streams is not 0 */
>  };
>  
> +struct sctp_add_streams {
> +	sctp_assoc_t sas_assoc_id;
> +	uint16_t sas_instrms;
> +	uint16_t sas_outstrms;
> +};
> +
>  #endif /* _UAPI_SCTP_H */
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 2c5c9ca..ae0a99e 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
>  	return retval;
>  }
>  
> +static int sctp_setsockopt_add_streams(struct sock *sk,
> +				       char __user *optval,
> +				       unsigned int optlen)
> +{
> +	struct sctp_association *asoc;
> +	struct sctp_add_streams params;
> +	int retval = -EINVAL;
> +
> +	if (optlen != sizeof(params))
> +		goto out;
> +
> +	if (copy_from_user(&params, optval, optlen)) {
> +		retval = -EFAULT;
> +		goto out;
> +	}
> +
> +	asoc = sctp_id2assoc(sk, params.sas_assoc_id);
> +	if (!asoc)
> +		goto out;
> +
> +	retval = sctp_send_add_streams(asoc, &params);
> +
> +out:
> +	return retval;
> +}
> +
>  /* API 6.2 setsockopt(), getsockopt()
>   *
>   * Applications use setsockopt() and getsockopt() to set or retrieve
> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>  	case SCTP_RESET_ASSOC:
>  		retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
>  		break;
> +	case SCTP_ADD_STREAMS:
> +		retval = sctp_setsockopt_add_streams(sk, optval, optlen);
> +		break;
>  	default:
>  		retval = -ENOPROTOOPT;
>  		break;
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index b368191..ba41837 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>  
>  	return retval;
>  }
> +
> +int sctp_send_add_streams(struct sctp_association *asoc,
> +			  struct sctp_add_streams *params)
> +{
> +	struct sctp_stream *stream = asoc->stream;
> +	struct sctp_chunk *chunk = NULL;
> +	int retval = -EINVAL;
> +	__u16 out, in;
> +
> +	if (!asoc->peer.reconf_capable ||
> +	    !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
> +		retval = -ENOPROTOOPT;
> +		goto out;
> +	}
> +
> +	if (asoc->strreset_outstanding) {
> +		retval = -EINPROGRESS;
> +		goto out;
> +	}
> +
> +	out = params->sas_outstrms;
> +	in  = params->sas_instrms;
> +
> +	if (!out && !in)
> +		goto out;
> +
> +	if (out) {
> +		__u16 nums = stream->outcnt + out;
> +
> +		/* Check for overflow, can't use nums here */
> +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> +			goto out;
> +
> +		/* Use ksize to check if stream array really needs to realloc */
> +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> +			struct sctp_stream_out *streamout;
> +
> +			streamout = kcalloc(nums, sizeof(*streamout),
> +					    GFP_KERNEL);
> +			if (!streamout) {
> +				retval = -ENOMEM;
> +				goto out;
> +			}
> +
> +			memcpy(streamout, stream->out,
> +			       sizeof(*streamout) * stream->outcnt);
> +
> +			kfree(stream->out);
> +			stream->out = streamout;
> +		}
> +
> +		stream->outcnt = nums;
> +	}
> +
> +	if (in) {
> +		__u16 nums = stream->incnt + in;
> +
> +		if (stream->incnt + in > SCTP_MAX_STREAM)
> +			goto out;
> +
> +		if (ksize(stream->in) / sizeof(*stream->in) < nums) {
> +			struct sctp_stream_in *streamin;
> +
> +			streamin = kcalloc(nums, sizeof(*streamin),
> +					   GFP_KERNEL);
> +			if (!streamin) {
> +				retval = -ENOMEM;
> +				goto out;
> +			}
> +
> +			memcpy(streamin, stream->in,
> +			       sizeof(*streamin) * stream->incnt);
> +
> +			kfree(stream->in);
> +			stream->in = streamin;
> +		}
> +
> +		stream->incnt = nums;
> +	}
> +
> +	chunk = sctp_make_strreset_addstrm(asoc, out, in);
> +	if (!chunk) {
> +		retval = -ENOMEM;
> +		goto out;
> +	}
> +
> +	asoc->strreset_outstanding = !!out + !!in;
> +	asoc->strreset_chunk = chunk;
> +	sctp_chunk_hold(asoc->strreset_chunk);
> +
> +	retval = sctp_send_reconf(asoc, chunk);
> +	if (retval) {
> +		sctp_chunk_put(asoc->strreset_chunk);
> +		asoc->strreset_chunk = NULL;

Similar comment on recovery steps here. If we are assuming we failed to
send the request, we should also revert the changes above.
The memory reallocation seems fine as the streams defaults to closed,
but shouldn't incnt and outcnt be reset?

> +	}
> +
> +out:
> +	return retval;
> +}
> -- 
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Xin Long]

On Fri, Jan 20, 2017 at 6:15 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 01:19:14AM +0800, Xin Long wrote:
>> This patch is to implement Sender-Side Procedures for the Add
>> Outgoing and Incoming Streams Request Parameter described in
>> rfc6525 section 5.1.5-5.1.6.
>>
>> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
>> 6.3.4 for users.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>>  include/net/sctp/sctp.h   |  2 +
>>  include/uapi/linux/sctp.h |  7 ++++
>>  net/sctp/socket.c         | 29 ++++++++++++++
>>  net/sctp/stream.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index b93820f..68ee1a6 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -199,6 +199,8 @@ int sctp_offload_init(void);
>>  int sctp_send_reset_streams(struct sctp_association *asoc,
>>                           struct sctp_reset_streams *params);
>>  int sctp_send_reset_assoc(struct sctp_association *asoc);
>> +int sctp_send_add_streams(struct sctp_association *asoc,
>> +                       struct sctp_add_streams *params);
>>
>>  /*
>>   * Module global variables
>> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>> index c0bd8c3..a91a9cc 100644
>> --- a/include/uapi/linux/sctp.h
>> +++ b/include/uapi/linux/sctp.h
>> @@ -118,6 +118,7 @@ typedef __s32 sctp_assoc_t;
>>  #define SCTP_ENABLE_STREAM_RESET     118
>>  #define SCTP_RESET_STREAMS   119
>>  #define SCTP_RESET_ASSOC     120
>> +#define SCTP_ADD_STREAMS     121
>>
>>  /* PR-SCTP policies */
>>  #define SCTP_PR_SCTP_NONE    0x0000
>> @@ -1027,4 +1028,10 @@ struct sctp_reset_streams {
>>       uint16_t srs_stream_list[];     /* list if srs_num_streams is not 0 */
>>  };
>>
>> +struct sctp_add_streams {
>> +     sctp_assoc_t sas_assoc_id;
>> +     uint16_t sas_instrms;
>> +     uint16_t sas_outstrms;
>> +};
>> +
>>  #endif /* _UAPI_SCTP_H */
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 2c5c9ca..ae0a99e 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3838,6 +3838,32 @@ static int sctp_setsockopt_reset_assoc(struct sock *sk,
>>       return retval;
>>  }
>>
>> +static int sctp_setsockopt_add_streams(struct sock *sk,
>> +                                    char __user *optval,
>> +                                    unsigned int optlen)
>> +{
>> +     struct sctp_association *asoc;
>> +     struct sctp_add_streams params;
>> +     int retval = -EINVAL;
>> +
>> +     if (optlen != sizeof(params))
>> +             goto out;
>> +
>> +     if (copy_from_user(&params, optval, optlen)) {
>> +             retval = -EFAULT;
>> +             goto out;
>> +     }
>> +
>> +     asoc = sctp_id2assoc(sk, params.sas_assoc_id);
>> +     if (!asoc)
>> +             goto out;
>> +
>> +     retval = sctp_send_add_streams(asoc, &params);
>> +
>> +out:
>> +     return retval;
>> +}
>> +
>>  /* API 6.2 setsockopt(), getsockopt()
>>   *
>>   * Applications use setsockopt() and getsockopt() to set or retrieve
>> @@ -4013,6 +4039,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>>       case SCTP_RESET_ASSOC:
>>               retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
>>               break;
>> +     case SCTP_ADD_STREAMS:
>> +             retval = sctp_setsockopt_add_streams(sk, optval, optlen);
>> +             break;
>>       default:
>>               retval = -ENOPROTOOPT;
>>               break;
>> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> index b368191..ba41837 100644
>> --- a/net/sctp/stream.c
>> +++ b/net/sctp/stream.c
>> @@ -195,3 +195,102 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>>
>>       return retval;
>>  }
>> +
>> +int sctp_send_add_streams(struct sctp_association *asoc,
>> +                       struct sctp_add_streams *params)
>> +{
>> +     struct sctp_stream *stream = asoc->stream;
>> +     struct sctp_chunk *chunk = NULL;
>> +     int retval = -EINVAL;
>> +     __u16 out, in;
>> +
>> +     if (!asoc->peer.reconf_capable ||
>> +         !(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) {
>> +             retval = -ENOPROTOOPT;
>> +             goto out;
>> +     }
>> +
>> +     if (asoc->strreset_outstanding) {
>> +             retval = -EINPROGRESS;
>> +             goto out;
>> +     }
>> +
>> +     out = params->sas_outstrms;
>> +     in  = params->sas_instrms;
>> +
>> +     if (!out && !in)
>> +             goto out;
>> +
>> +     if (out) {
>> +             __u16 nums = stream->outcnt + out;
>> +
>> +             /* Check for overflow, can't use nums here */
>> +             if (stream->outcnt + out > SCTP_MAX_STREAM)
>> +                     goto out;
>> +
>> +             /* Use ksize to check if stream array really needs to realloc */
>> +             if (ksize(stream->out) / sizeof(*stream->out) < nums) {
>> +                     struct sctp_stream_out *streamout;
>> +
>> +                     streamout = kcalloc(nums, sizeof(*streamout),
>> +                                         GFP_KERNEL);
>> +                     if (!streamout) {
>> +                             retval = -ENOMEM;
>> +                             goto out;
>> +                     }
>> +
>> +                     memcpy(streamout, stream->out,
>> +                            sizeof(*streamout) * stream->outcnt);
>> +
>> +                     kfree(stream->out);
>> +                     stream->out = streamout;
>> +             }
>> +
>> +             stream->outcnt = nums;
>> +     }
>> +
>> +     if (in) {
>> +             __u16 nums = stream->incnt + in;
>> +
>> +             if (stream->incnt + in > SCTP_MAX_STREAM)
>> +                     goto out;
>> +
>> +             if (ksize(stream->in) / sizeof(*stream->in) < nums) {
>> +                     struct sctp_stream_in *streamin;
>> +
>> +                     streamin = kcalloc(nums, sizeof(*streamin),
>> +                                        GFP_KERNEL);
>> +                     if (!streamin) {
>> +                             retval = -ENOMEM;
>> +                             goto out;
>> +                     }
>> +
>> +                     memcpy(streamin, stream->in,
>> +                            sizeof(*streamin) * stream->incnt);
>> +
>> +                     kfree(stream->in);
>> +                     stream->in = streamin;
>> +             }
>> +
>> +             stream->incnt = nums;
>> +     }
>> +
>> +     chunk = sctp_make_strreset_addstrm(asoc, out, in);
>> +     if (!chunk) {
>> +             retval = -ENOMEM;
>> +             goto out;
>> +     }
>> +
>> +     asoc->strreset_outstanding = !!out + !!in;
>> +     asoc->strreset_chunk = chunk;
>> +     sctp_chunk_hold(asoc->strreset_chunk);
>> +
>> +     retval = sctp_send_reconf(asoc, chunk);
>> +     if (retval) {
>> +             sctp_chunk_put(asoc->strreset_chunk);
>> +             asoc->strreset_chunk = NULL;
>
> Similar comment on recovery steps here. If we are assuming we failed to
> send the request, we should also revert the changes above.
> The memory reallocation seems fine as the streams defaults to closed,
> but shouldn't incnt and outcnt be reset?
yeah, I will set  incnt and outcnt only when retval is 0.

>
>> +     }
>> +
>> +out:
>> +     return retval;
>> +}
>> --
>> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[David Laight]

From: Xin Long
> Sent: 19 January 2017 17:19
> This patch is to implement Sender-Side Procedures for the Add
> Outgoing and Incoming Streams Request Parameter described in
> rfc6525 section 5.1.5-5.1.6.
> 
> It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> 6.3.4 for users.
...
> +	out = params->sas_outstrms;
> +	in  = params->sas_instrms;
> +
> +	if (!out && !in)
> +		goto out;
> +
> +	if (out) {
> +		__u16 nums = stream->outcnt + out;

Make nums 'unsigned int', the code will be smaller and you can
use the value for the overflow check.

> +		/* Check for overflow, can't use nums here */
> +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> +			goto out;
> +
> +		/* Use ksize to check if stream array really needs to realloc */
> +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> +			struct sctp_stream_out *streamout;
> +
> +			streamout = kcalloc(nums, sizeof(*streamout),
> +					    GFP_KERNEL);
> +			if (!streamout) {
> +				retval = -ENOMEM;
> +				goto out;
> +			}
> +
> +			memcpy(streamout, stream->out,
> +			       sizeof(*streamout) * stream->outcnt);
> +
> +			kfree(stream->out);
> +			stream->out = streamout;
> +		}

Does kcalloc() zero the entire area, or just the length you ask for?
If the latter you need to zero the rest here.
...

	David

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Neil Horman]

On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> From: Xin Long
> > Sent: 19 January 2017 17:19
> > This patch is to implement Sender-Side Procedures for the Add
> > Outgoing and Incoming Streams Request Parameter described in
> > rfc6525 section 5.1.5-5.1.6.
> > 
> > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > 6.3.4 for users.
> ...
> > +	out = params->sas_outstrms;
> > +	in  = params->sas_instrms;
> > +
> > +	if (!out && !in)
> > +		goto out;
> > +
> > +	if (out) {
> > +		__u16 nums = stream->outcnt + out;
> 
> Make nums 'unsigned int', the code will be smaller and you can
> use the value for the overflow check.
> 
> > +		/* Check for overflow, can't use nums here */
> > +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> > +			goto out;
> > +
> > +		/* Use ksize to check if stream array really needs to realloc */
> > +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> > +			struct sctp_stream_out *streamout;
> > +
> > +			streamout = kcalloc(nums, sizeof(*streamout),
> > +					    GFP_KERNEL);
> > +			if (!streamout) {
> > +				retval = -ENOMEM;
> > +				goto out;
> > +			}
> > +
> > +			memcpy(streamout, stream->out,
> > +			       sizeof(*streamout) * stream->outcnt);
> > +
> > +			kfree(stream->out);
> > +			stream->out = streamout;
> > +		}
> 
> Does kcalloc() zero the entire area, or just the length you ask for?
> If the latter you need to zero the rest here.
Better still, just use krealloc.  You still need to zero out any space beyond
the old length, but it will make the code shorter, and avoid the need for
additional temporary variables.

Neil

> ...
> 
> 	David
> 
> --
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Mon, Jan 23, 2017 at 09:53:47AM -0500, Neil Horman wrote:
> On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 19 January 2017 17:19
> > > This patch is to implement Sender-Side Procedures for the Add
> > > Outgoing and Incoming Streams Request Parameter described in
> > > rfc6525 section 5.1.5-5.1.6.
> > > 
> > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > 6.3.4 for users.
> > ...
> > > +	out = params->sas_outstrms;
> > > +	in  = params->sas_instrms;
> > > +
> > > +	if (!out && !in)
> > > +		goto out;
> > > +
> > > +	if (out) {
> > > +		__u16 nums = stream->outcnt + out;
> > 
> > Make nums 'unsigned int', the code will be smaller and you can
> > use the value for the overflow check.
> > 
> > > +		/* Check for overflow, can't use nums here */
> > > +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > +			goto out;
> > > +
> > > +		/* Use ksize to check if stream array really needs to realloc */
> > > +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> > > +			struct sctp_stream_out *streamout;
> > > +
> > > +			streamout = kcalloc(nums, sizeof(*streamout),
> > > +					    GFP_KERNEL);
> > > +			if (!streamout) {
> > > +				retval = -ENOMEM;
> > > +				goto out;
> > > +			}
> > > +
> > > +			memcpy(streamout, stream->out,
> > > +			       sizeof(*streamout) * stream->outcnt);
> > > +
> > > +			kfree(stream->out);
> > > +			stream->out = streamout;
> > > +		}
> > 
> > Does kcalloc() zero the entire area, or just the length you ask for?
> > If the latter you need to zero the rest here.
> Better still, just use krealloc.  You still need to zero out any space beyond
> the old length, but it will make the code shorter, and avoid the need for
> additional temporary variables.

Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
slab for us before doing the memcpy.
I didn't follow all paths but in slab_alloc_node it will end up calling:
        if (unlikely(gfpflags & __GFP_ZERO) && object)
                memset(object, 0, s->object_size);
So I would expect that other paths also do it.

  Marcelo

> 
> Neil
> 
> > ...
> > 
> > 	David
> > 
> > --
> > 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
> > 
> --
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 16:03
...
> > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > If the latter you need to zero the rest here.
> > Better still, just use krealloc.  You still need to zero out any space beyond
> > the old length, but it will make the code shorter, and avoid the need for
> > additional temporary variables.
> 
> Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> slab for us before doing the memcpy.
> I didn't follow all paths but in slab_alloc_node it will end up calling:
>         if (unlikely(gfpflags & __GFP_ZERO) && object)
>                 memset(object, 0, s->object_size);
> So I would expect that other paths also do it.

You probably don't want krealloc() zeroing all of the new area.

	David

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

['Marcelo Ricardo Leitner']

On Tue, Jan 24, 2017 at 12:35:39PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 16:03
> ...
> > > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > > If the latter you need to zero the rest here.
> > > Better still, just use krealloc.  You still need to zero out any space beyond
> > > the old length, but it will make the code shorter, and avoid the need for
> > > additional temporary variables.
> > 
> > Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> > slab for us before doing the memcpy.
> > I didn't follow all paths but in slab_alloc_node it will end up calling:
> >         if (unlikely(gfpflags & __GFP_ZERO) && object)
> >                 memset(object, 0, s->object_size);
> > So I would expect that other paths also do it.
> 
> You probably don't want krealloc() zeroing all of the new area.

Yep, agreed.

  Marcelo

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[Marcelo Ricardo Leitner]

On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> From: Xin Long
> > Sent: 19 January 2017 17:19
> > This patch is to implement Sender-Side Procedures for the Add
> > Outgoing and Incoming Streams Request Parameter described in
> > rfc6525 section 5.1.5-5.1.6.
> > 
> > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > 6.3.4 for users.
> ...
> > +	out = params->sas_outstrms;
> > +	in  = params->sas_instrms;
> > +
> > +	if (!out && !in)
> > +		goto out;
> > +
> > +	if (out) {
> > +		__u16 nums = stream->outcnt + out;
> 
> Make nums 'unsigned int', the code will be smaller and you can
> use the value for the overflow check.

Smaller as in to avoid the sum below?

> 
> > +		/* Check for overflow, can't use nums here */
> > +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> > +			goto out;
> > +
> > +		/* Use ksize to check if stream array really needs to realloc */
> > +		if (ksize(stream->out) / sizeof(*stream->out) < nums) {
> > +			struct sctp_stream_out *streamout;
> > +
> > +			streamout = kcalloc(nums, sizeof(*streamout),
> > +					    GFP_KERNEL);
> > +			if (!streamout) {
> > +				retval = -ENOMEM;
> > +				goto out;
> > +			}
> > +
> > +			memcpy(streamout, stream->out,
> > +			       sizeof(*streamout) * stream->outcnt);
> > +
> > +			kfree(stream->out);
> > +			stream->out = streamout;
> > +		}
> 
> Does kcalloc() zero the entire area, or just the length you ask for?
> If the latter you need to zero the rest here.
> ...
> 
> 	David
> 
> --
> 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: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 18:48
> On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 19 January 2017 17:19
> > > This patch is to implement Sender-Side Procedures for the Add
> > > Outgoing and Incoming Streams Request Parameter described in
> > > rfc6525 section 5.1.5-5.1.6.
> > >
> > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > 6.3.4 for users.
> > ...
> > > +	out = params->sas_outstrms;
> > > +	in  = params->sas_instrms;
> > > +
> > > +	if (!out && !in)
> > > +		goto out;
> > > +
> > > +	if (out) {
> > > +		__u16 nums = stream->outcnt + out;
> >
> > Make nums 'unsigned int', the code will be smaller and you can
> > use the value for the overflow check.
> 
> Smaller as in to avoid the sum below?
> 
> >
> > > +		/* Check for overflow, can't use nums here */
> > > +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > +			goto out;

No, smaller as in not requiring the compiler to add instructions
to mask (or worse sign extend) the result of the arithmetic expression
to less than the number of bits in an 'int' when the result of the
expression is to be kept in a register.

The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
For everything else you really don't want to do arithmetic on char
and short unless you really want the wrapping to happen.

	David

Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams

['Marcelo Ricardo Leitner']

On Tue, Jan 24, 2017 at 12:34:06PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 18:48
> > On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 19 January 2017 17:19
> > > > This patch is to implement Sender-Side Procedures for the Add
> > > > Outgoing and Incoming Streams Request Parameter described in
> > > > rfc6525 section 5.1.5-5.1.6.
> > > >
> > > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > > 6.3.4 for users.
> > > ...
> > > > +	out = params->sas_outstrms;
> > > > +	in  = params->sas_instrms;
> > > > +
> > > > +	if (!out && !in)
> > > > +		goto out;
> > > > +
> > > > +	if (out) {
> > > > +		__u16 nums = stream->outcnt + out;
> > >
> > > Make nums 'unsigned int', the code will be smaller and you can
> > > use the value for the overflow check.
> > 
> > Smaller as in to avoid the sum below?
> > 
> > >
> > > > +		/* Check for overflow, can't use nums here */
> > > > +		if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > > +			goto out;
> 
> No, smaller as in not requiring the compiler to add instructions
> to mask (or worse sign extend) the result of the arithmetic expression
> to less than the number of bits in an 'int' when the result of the
> expression is to be kept in a register.
> 
> The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
> For everything else you really don't want to do arithmetic on char
> and short unless you really want the wrapping to happen.

Okay thanks.

  Marcelo

RE: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Laight]

From: Xin Long
> Sent: 19 January 2017 17:19
> This patch is to define Add Incoming/Outgoing Streams Request
> Parameter described in rfc6525 section 4.5 and 4.6. They can
> be in one same chunk trunk as rfc6525 section 3.1-7 describes,
> so make them in one function.
...
> +struct sctp_strreset_addstrm {
> +	sctp_paramhdr_t param_hdr;
> +	__u32 request_seq;
> +	__u16 number_of_streams;
> +	__u16 reserved;
> +} __packed;
...
> +		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
> +		addstrm.param_hdr.length = htons(size);
> +		addstrm.number_of_streams = htons(out);
> +		addstrm.request_seq = htonl(asoc->strreset_outseq);
> +		addstrm.reserved = 0;
> +
> +		sctp_addto_chunk(retval, size, &addstrm);

Since you allocate the sctp_strreset_addstrm structure on stack
there is no requirement for it to be packed.

	David

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote:
> From: Xin Long
> > Sent: 19 January 2017 17:19
> > This patch is to define Add Incoming/Outgoing Streams Request
> > Parameter described in rfc6525 section 4.5 and 4.6. They can
> > be in one same chunk trunk as rfc6525 section 3.1-7 describes,
> > so make them in one function.
> ...
> > +struct sctp_strreset_addstrm {
> > +	sctp_paramhdr_t param_hdr;
> > +	__u32 request_seq;
> > +	__u16 number_of_streams;
> > +	__u16 reserved;
> > +} __packed;
> ...
> > +		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
> > +		addstrm.param_hdr.length = htons(size);
> > +		addstrm.number_of_streams = htons(out);
> > +		addstrm.request_seq = htonl(asoc->strreset_outseq);
> > +		addstrm.reserved = 0;
> > +
> > +		sctp_addto_chunk(retval, size, &addstrm);
> 
> Since you allocate the sctp_strreset_addstrm structure on stack
> there is no requirement for it to be packed.

It shouldn't matter that it's allocated on stack. Why should it?
We need it to be packed as this is a header that will be sent out to
another peer, so there can't be any padding on it.

  Marcelo

RE: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Laight]

From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39
> To: David Laight
> On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 19 January 2017 17:19
> > > This patch is to define Add Incoming/Outgoing Streams Request
> > > Parameter described in rfc6525 section 4.5 and 4.6. They can
> > > be in one same chunk trunk as rfc6525 section 3.1-7 describes,
> > > so make them in one function.
> > ...
> > > +struct sctp_strreset_addstrm {
> > > +	sctp_paramhdr_t param_hdr;
> > > +	__u32 request_seq;
> > > +	__u16 number_of_streams;
> > > +	__u16 reserved;
> > > +} __packed;
> > ...
> > > +		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
> > > +		addstrm.param_hdr.length = htons(size);
> > > +		addstrm.number_of_streams = htons(out);
> > > +		addstrm.request_seq = htonl(asoc->strreset_outseq);
> > > +		addstrm.reserved = 0;
> > > +
> > > +		sctp_addto_chunk(retval, size, &addstrm);
> >
> > Since you allocate the sctp_strreset_addstrm structure on stack
> > there is no requirement for it to be packed.
> 
> It shouldn't matter that it's allocated on stack. Why should it?
> We need it to be packed as this is a header that will be sent out to
> another peer, so there can't be any padding on it.

That isn't what __packed means.
It means that the compiler must assume that the structure can be
misaligned in memory and must use byte memory accesses on systems
that fault misaligned memory accesses.

	David

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

['Marcelo Ricardo Leitner']

On Mon, Jan 23, 2017 at 12:26:12PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39
> > To: David Laight
> > On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 19 January 2017 17:19
> > > > This patch is to define Add Incoming/Outgoing Streams Request
> > > > Parameter described in rfc6525 section 4.5 and 4.6. They can
> > > > be in one same chunk trunk as rfc6525 section 3.1-7 describes,
> > > > so make them in one function.
> > > ...
> > > > +struct sctp_strreset_addstrm {
> > > > +	sctp_paramhdr_t param_hdr;
> > > > +	__u32 request_seq;
> > > > +	__u16 number_of_streams;
> > > > +	__u16 reserved;
> > > > +} __packed;
> > > ...
> > > > +		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
> > > > +		addstrm.param_hdr.length = htons(size);
> > > > +		addstrm.number_of_streams = htons(out);
> > > > +		addstrm.request_seq = htonl(asoc->strreset_outseq);
> > > > +		addstrm.reserved = 0;
> > > > +
> > > > +		sctp_addto_chunk(retval, size, &addstrm);
> > >
> > > Since you allocate the sctp_strreset_addstrm structure on stack
> > > there is no requirement for it to be packed.
> > 
> > It shouldn't matter that it's allocated on stack. Why should it?
> > We need it to be packed as this is a header that will be sent out to
> > another peer, so there can't be any padding on it.
> 
> That isn't what __packed means.
> It means that the compiler must assume that the structure can be
> misaligned in memory and must use byte memory accesses on systems
> that fault misaligned memory accesses.

That's a side-effect of it.

https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes
"This attribute, attached to struct or union type definition, specifies
that each member (other than zero-width bit-fields) of the structure or
union is placed to minimize the memory required. "

So, no padding. A field just after the other, which is what we want on a
network header.

  Marcelo

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Miller]

From: "'Marcelo Ricardo Leitner'" <marcelo.leitner@gmail.com>
Date: Mon, 23 Jan 2017 10:36:28 -0200

> So, no padding. A field just after the other, which is what we want on a
> network header.

It isn't necessary!

Show me a case where it is required when you use properly fixed sized
types and a proper ordering of the struct members.  No padding is
going in there, go and check.

Do we splatter __packed all over our ipv4/ipv6 header, TCP header, UDP
header, etc. structures?  No, we don't because it's totally unecessary.

I will not accept __packed being used unless it is absolutely, provably,
the only way to solve a particular problem.  And when that does happen,
I am going to require a huge comment explaining in detail why this is
the case, and why no other approach or solution solved the problem.

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[marcelo.leitner]

On Mon, Jan 23, 2017 at 11:00:47AM -0500, David Miller wrote:
> From: "'Marcelo Ricardo Leitner'" <marcelo.leitner@gmail.com>
> Date: Mon, 23 Jan 2017 10:36:28 -0200
> 
> > So, no padding. A field just after the other, which is what we want on a
> > network header.
> 
> It isn't necessary!
> 
> Show me a case where it is required when you use properly fixed sized
> types and a proper ordering of the struct members.  No padding is
> going in there, go and check.
> 
> Do we splatter __packed all over our ipv4/ipv6 header, TCP header, UDP
> header, etc. structures?  No, we don't because it's totally unecessary.

Err, sure, right.

> 
> I will not accept __packed being used unless it is absolutely, provably,
> the only way to solve a particular problem.  And when that does happen,
> I am going to require a huge comment explaining in detail why this is
> the case, and why no other approach or solution solved the problem.

Would this be a candidate for checkpatch.pl?

  Marcelo

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Miller]

From: marcelo.leitner@gmail.com
Date: Mon, 23 Jan 2017 14:14:58 -0200

> On Mon, Jan 23, 2017 at 11:00:47AM -0500, David Miller wrote:
>> I will not accept __packed being used unless it is absolutely, provably,
>> the only way to solve a particular problem.  And when that does happen,
>> I am going to require a huge comment explaining in detail why this is
>> the case, and why no other approach or solution solved the problem.
> 
> Would this be a candidate for checkpatch.pl?

If this helps developers, sure.

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 23 Jan 2017 12:26:12 +0000

> From: Marcelo Ricardo Leitner > Sent: 20 January 2017 16:39
>> To: David Laight
>> On Fri, Jan 20, 2017 at 02:50:01PM +0000, David Laight wrote:
>> > From: Xin Long
>> > > Sent: 19 January 2017 17:19
>> > > This patch is to define Add Incoming/Outgoing Streams Request
>> > > Parameter described in rfc6525 section 4.5 and 4.6. They can
>> > > be in one same chunk trunk as rfc6525 section 3.1-7 describes,
>> > > so make them in one function.
>> > ...
>> > > +struct sctp_strreset_addstrm {
>> > > +	sctp_paramhdr_t param_hdr;
>> > > +	__u32 request_seq;
>> > > +	__u16 number_of_streams;
>> > > +	__u16 reserved;
>> > > +} __packed;
>> > ...
>> > > +		addstrm.param_hdr.type = SCTP_PARAM_RESET_ADD_OUT_STREAMS;
>> > > +		addstrm.param_hdr.length = htons(size);
>> > > +		addstrm.number_of_streams = htons(out);
>> > > +		addstrm.request_seq = htonl(asoc->strreset_outseq);
>> > > +		addstrm.reserved = 0;
>> > > +
>> > > +		sctp_addto_chunk(retval, size, &addstrm);
>> >
>> > Since you allocate the sctp_strreset_addstrm structure on stack
>> > there is no requirement for it to be packed.
>> 
>> It shouldn't matter that it's allocated on stack. Why should it?
>> We need it to be packed as this is a header that will be sent out to
>> another peer, so there can't be any padding on it.
> 
> That isn't what __packed means.
> It means that the compiler must assume that the structure can be
> misaligned in memory and must use byte memory accesses on systems
> that fault misaligned memory accesses.

Also, for the types involved there will not be any padding at all.
Check if you do not believe me.

If this is "so critical" for end to end communication, why the heck
do you not see __packed sprinkled all over our definitions for IPV4,
IPV6, TCP, UDP, etc. headers?

Do you know why?  Because it's completely unnecessary...

Can I seriously, strongly, state that people should use __packed as
the last possible resort when writing code?

Don't add it unless you run into a problem which fundamentally cannot
be solved by either using different data types, a different ordering
of data items, or similar.

And when __packed is actually legitimate and used I am now going to
require a HUGE FRIGGIN' COMMENT explaining why no other approach
whatsoever can solve the problem.

__packed is going to emit incredibly inefficient code on some cpus,
I really don't think people truly understand the enormous negative
ramifications of using __packed.

It is almost never, ever, necessary.

And I'm tired of writing this tirade every half year or so.

__packed, just say no...

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[marcelo.leitner]

On Mon, Jan 23, 2017 at 10:58:10AM -0500, David Miller wrote:
> If this is "so critical" for end to end communication, why the heck
> do you not see __packed sprinkled all over our definitions for IPV4,
> IPV6, TCP, UDP, etc. headers?
> 
> Do you know why?  Because it's completely unnecessary...

Btw, virtually all sctp headers are currently tagged with __packed,
since forever.

$ git grep __packed -- include/linux/sctp.h
include/linux/sctp.h:} __packed sctp_sctphdr_t;
include/linux/sctp.h:} __packed sctp_chunkhdr_t;
include/linux/sctp.h:} __packed sctp_paramhdr_t;
include/linux/sctp.h:} __packed sctp_datahdr_t;
include/linux/sctp.h:} __packed sctp_data_chunk_t;
include/linux/sctp.h:} __packed sctp_inithdr_t;
include/linux/sctp.h:} __packed sctp_init_chunk_t;
include/linux/sctp.h:} __packed sctp_ipv4addr_param_t;
include/linux/sctp.h:} __packed sctp_ipv6addr_param_t;
include/linux/sctp.h:} __packed sctp_cookie_preserve_param_t;
include/linux/sctp.h:} __packed sctp_hostname_param_t;
include/linux/sctp.h:} __packed sctp_supported_addrs_param_t;
include/linux/sctp.h:} __packed sctp_ecn_capable_param_t;
include/linux/sctp.h:} __packed sctp_adaptation_ind_param_t;
include/linux/sctp.h:} __packed sctp_supported_ext_param_t;
include/linux/sctp.h:} __packed sctp_random_param_t;
include/linux/sctp.h:} __packed sctp_chunks_param_t;
include/linux/sctp.h:} __packed sctp_hmac_algo_param_t;
include/linux/sctp.h:} __packed sctp_cookie_param_t;
include/linux/sctp.h:} __packed sctp_unrecognized_param_t;
include/linux/sctp.h:} __packed sctp_gap_ack_block_t;
include/linux/sctp.h:} __packed sctp_sackhdr_t;
include/linux/sctp.h:} __packed sctp_sack_chunk_t;
include/linux/sctp.h:} __packed sctp_heartbeathdr_t;
include/linux/sctp.h:} __packed sctp_heartbeat_chunk_t;
include/linux/sctp.h:} __packed sctp_abort_chunk_t;
include/linux/sctp.h:} __packed sctp_shutdownhdr_t;
include/linux/sctp.h:} __packed;
include/linux/sctp.h:} __packed sctp_errhdr_t;
include/linux/sctp.h:} __packed sctp_operr_chunk_t;
include/linux/sctp.h:} __packed sctp_ecne_chunk_t;
include/linux/sctp.h:} __packed sctp_cwr_chunk_t;
...

^1da177e4c3f4 (Linus Torvalds           2005-04-16 15:20:36 -0700  64) }
__attribute__((packed)) sctp_sctphdr_t;

I'm reviewing them all and will probably post a patch to remove them.

  Marcelo

Re: [PATCHv3 net-next 3/4] sctp: add support for generating stream reconf add incoming/outgoing stre

[David Miller]

From: marcelo.leitner@gmail.com
Date: Sun, 29 Jan 2017 12:31:17 -0200

> On Mon, Jan 23, 2017 at 10:58:10AM -0500, David Miller wrote:
>> If this is "so critical" for end to end communication, why the heck
>> do you not see __packed sprinkled all over our definitions for IPV4,
>> IPV6, TCP, UDP, etc. headers?
>> 
>> Do you know why?  Because it's completely unnecessary...
> 
> Btw, virtually all sctp headers are currently tagged with __packed,
> since forever.
 ...
> I'm reviewing them all and will probably post a patch to remove them.

Thanks.

Re: [PATCHv3 net-next 2/4] sctp: implement sender-side procedures for SSN/TSN Reset Request Paramete

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 01:19:12AM +0800, Xin Long wrote:
> This patch is to implement Sender-Side Procedures for the SSN/TSN
> Reset Request Parameter descibed in rfc6525 section 5.1.4.
> 
> It is also to add sockopt SCTP_RESET_ASSOC in rfc6525 section 6.3.3
> for users.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/sctp/sctp.h   |  1 +
>  include/uapi/linux/sctp.h |  1 +
>  net/sctp/socket.c         | 29 +++++++++++++++++++++++++++++
>  net/sctp/stream.c         | 33 +++++++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 3cfd365b..b93820f 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -198,6 +198,7 @@ int sctp_offload_init(void);
>   */
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params);
> +int sctp_send_reset_assoc(struct sctp_association *asoc);
>  
>  /*
>   * Module global variables
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index 03c27ce..c0bd8c3 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -117,6 +117,7 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_PR_ASSOC_STATUS	115
>  #define SCTP_ENABLE_STREAM_RESET	118
>  #define SCTP_RESET_STREAMS	119
> +#define SCTP_RESET_ASSOC	120
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bee4dd3..2c5c9ca 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3812,6 +3812,32 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
>  	return retval;
>  }
>  
> +static int sctp_setsockopt_reset_assoc(struct sock *sk,
> +				       char __user *optval,
> +				       unsigned int optlen)
> +{
> +	struct sctp_association *asoc;
> +	sctp_assoc_t associd;
> +	int retval = -EINVAL;
> +
> +	if (optlen != sizeof(associd))
> +		goto out;
> +
> +	if (copy_from_user(&associd, optval, optlen)) {
> +		retval = -EFAULT;
> +		goto out;
> +	}
> +
> +	asoc = sctp_id2assoc(sk, associd);
> +	if (!asoc)
> +		goto out;
> +
> +	retval = sctp_send_reset_assoc(asoc);
> +
> +out:
> +	return retval;
> +}
> +
>  /* API 6.2 setsockopt(), getsockopt()
>   *
>   * Applications use setsockopt() and getsockopt() to set or retrieve
> @@ -3984,6 +4010,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>  	case SCTP_RESET_STREAMS:
>  		retval = sctp_setsockopt_reset_streams(sk, optval, optlen);
>  		break;
> +	case SCTP_RESET_ASSOC:
> +		retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
> +		break;
>  	default:
>  		retval = -ENOPROTOOPT;
>  		break;
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 13d5e07..b368191 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -162,3 +162,36 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>  out:
>  	return retval;
>  }
> +
> +int sctp_send_reset_assoc(struct sctp_association *asoc)
> +{
> +	struct sctp_chunk *chunk = NULL;
> +	int retval;
> +	__u16 i;
> +
> +	if (!asoc->peer.reconf_capable ||
> +	    !(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
> +		return -ENOPROTOOPT;
> +
> +	if (asoc->strreset_outstanding)
> +		return -EINPROGRESS;
> +
> +	chunk = sctp_make_strreset_tsnreq(asoc);
> +	if (!chunk)
> +		return -ENOMEM;
> +

Please add a comment here explaining that the for below is to block
further xmit of data until this request is completed.

> +	for (i = 0; i < asoc->stream->outcnt; i++)
> +		asoc->stream->out[i].state = SCTP_STREAM_CLOSED;
> +
> +	asoc->strreset_outstanding = 1;
> +	asoc->strreset_chunk = chunk;
> +	sctp_chunk_hold(asoc->strreset_chunk);
> +
> +	retval = sctp_send_reconf(asoc, chunk);
> +	if (retval) {
> +		sctp_chunk_put(asoc->strreset_chunk);
> +		asoc->strreset_chunk = NULL;

If this happens, the asoc will get stuck, as strreset_outstanding is
marked as 1, all streams are closed and no packet was even queued
(I'm assuming so as you're dropping the reference on it).


> +	}
> +
> +	return retval;
> +}
> -- 
> 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: [PATCHv3 net-next 2/4] sctp: implement sender-side procedures for SSN/TSN Reset Request Paramete

[Xin Long]

On Fri, Jan 20, 2017 at 6:02 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 01:19:12AM +0800, Xin Long wrote:
>> This patch is to implement Sender-Side Procedures for the SSN/TSN
>> Reset Request Parameter descibed in rfc6525 section 5.1.4.
>>
>> It is also to add sockopt SCTP_RESET_ASSOC in rfc6525 section 6.3.3
>> for users.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>>  include/net/sctp/sctp.h   |  1 +
>>  include/uapi/linux/sctp.h |  1 +
>>  net/sctp/socket.c         | 29 +++++++++++++++++++++++++++++
>>  net/sctp/stream.c         | 33 +++++++++++++++++++++++++++++++++
>>  4 files changed, 64 insertions(+)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index 3cfd365b..b93820f 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -198,6 +198,7 @@ int sctp_offload_init(void);
>>   */
>>  int sctp_send_reset_streams(struct sctp_association *asoc,
>>                           struct sctp_reset_streams *params);
>> +int sctp_send_reset_assoc(struct sctp_association *asoc);
>>
>>  /*
>>   * Module global variables
>> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>> index 03c27ce..c0bd8c3 100644
>> --- a/include/uapi/linux/sctp.h
>> +++ b/include/uapi/linux/sctp.h
>> @@ -117,6 +117,7 @@ typedef __s32 sctp_assoc_t;
>>  #define SCTP_PR_ASSOC_STATUS 115
>>  #define SCTP_ENABLE_STREAM_RESET     118
>>  #define SCTP_RESET_STREAMS   119
>> +#define SCTP_RESET_ASSOC     120
>>
>>  /* PR-SCTP policies */
>>  #define SCTP_PR_SCTP_NONE    0x0000
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index bee4dd3..2c5c9ca 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3812,6 +3812,32 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
>>       return retval;
>>  }
>>
>> +static int sctp_setsockopt_reset_assoc(struct sock *sk,
>> +                                    char __user *optval,
>> +                                    unsigned int optlen)
>> +{
>> +     struct sctp_association *asoc;
>> +     sctp_assoc_t associd;
>> +     int retval = -EINVAL;
>> +
>> +     if (optlen != sizeof(associd))
>> +             goto out;
>> +
>> +     if (copy_from_user(&associd, optval, optlen)) {
>> +             retval = -EFAULT;
>> +             goto out;
>> +     }
>> +
>> +     asoc = sctp_id2assoc(sk, associd);
>> +     if (!asoc)
>> +             goto out;
>> +
>> +     retval = sctp_send_reset_assoc(asoc);
>> +
>> +out:
>> +     return retval;
>> +}
>> +
>>  /* API 6.2 setsockopt(), getsockopt()
>>   *
>>   * Applications use setsockopt() and getsockopt() to set or retrieve
>> @@ -3984,6 +4010,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>>       case SCTP_RESET_STREAMS:
>>               retval = sctp_setsockopt_reset_streams(sk, optval, optlen);
>>               break;
>> +     case SCTP_RESET_ASSOC:
>> +             retval = sctp_setsockopt_reset_assoc(sk, optval, optlen);
>> +             break;
>>       default:
>>               retval = -ENOPROTOOPT;
>>               break;
>> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> index 13d5e07..b368191 100644
>> --- a/net/sctp/stream.c
>> +++ b/net/sctp/stream.c
>> @@ -162,3 +162,36 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>>  out:
>>       return retval;
>>  }
>> +
>> +int sctp_send_reset_assoc(struct sctp_association *asoc)
>> +{
>> +     struct sctp_chunk *chunk = NULL;
>> +     int retval;
>> +     __u16 i;
>> +
>> +     if (!asoc->peer.reconf_capable ||
>> +         !(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
>> +             return -ENOPROTOOPT;
>> +
>> +     if (asoc->strreset_outstanding)
>> +             return -EINPROGRESS;
>> +
>> +     chunk = sctp_make_strreset_tsnreq(asoc);
>> +     if (!chunk)
>> +             return -ENOMEM;
>> +
>
> Please add a comment here explaining that the for below is to block
> further xmit of data until this request is completed.
sure.

>
>> +     for (i = 0; i < asoc->stream->outcnt; i++)
>> +             asoc->stream->out[i].state = SCTP_STREAM_CLOSED;
>> +
>> +     asoc->strreset_outstanding = 1;
>> +     asoc->strreset_chunk = chunk;
>> +     sctp_chunk_hold(asoc->strreset_chunk);
>> +
>> +     retval = sctp_send_reconf(asoc, chunk);
>> +     if (retval) {
>> +             sctp_chunk_put(asoc->strreset_chunk);
>> +             asoc->strreset_chunk = NULL;
>
> If this happens, the asoc will get stuck, as strreset_outstanding is
> marked as 1, all streams are closed and no packet was even queued
> (I'm assuming so as you're dropping the reference on it).
you're right, I'm planning to move:
>> +     for (i = 0; i < asoc->stream->outcnt; i++)
>> +             asoc->stream->out[i].state = SCTP_STREAM_CLOSED;
>> +
>> +     asoc->strreset_outstanding = 1;

right after this check. so that it will do it only when
the reconf chunk is sent out.

>
>
>> +     }
>> +
>> +     return retval;
>> +}
>> --
>> 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
>>