| Message ID | daa150a23548dda0a2fd513c622773f0ff9b2df3.1484845510.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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(¶ms, optval, optlen)) { > + retval = -EFAULT; > + goto out; > + } > + > + asoc = sctp_id2assoc(sk, params.sas_assoc_id); > + if (!asoc) > + goto out; > + > + retval = sctp_send_add_streams(asoc, ¶ms); > + > +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 >
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(¶ms, optval, optlen)) { > + retval = -EFAULT; > + goto out; > + } > + > + asoc = sctp_id2assoc(sk, params.sas_assoc_id); > + if (!asoc) > + goto out; > + > + retval = sctp_send_add_streams(asoc, ¶ms); > + > +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 >
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
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(¶ms, optval, optlen)) { >> + retval = -EFAULT; >> + goto out; >> + } >> + >> + asoc = sctp_id2assoc(sk, params.sas_assoc_id); >> + if (!asoc) >> + goto out; >> + >> + retval = sctp_send_add_streams(asoc, ¶ms); >> + >> +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 >>
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(¶ms, optval, optlen)) { >> + retval = -EFAULT; >> + goto out; >> + } >> + >> + asoc = sctp_id2assoc(sk, params.sas_assoc_id); >> + if (!asoc) >> + goto out; >> + >> + retval = sctp_send_add_streams(asoc, ¶ms); >> + >> +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 >>
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(¶ms, optval, optlen)) { > >> + retval = -EFAULT; > >> + goto out; > >> + } > >> + > >> + asoc = sctp_id2assoc(sk, params.sas_assoc_id); > >> + if (!asoc) > >> + goto out; > >> + > >> + retval = sctp_send_add_streams(asoc, ¶ms); > >> + > >> +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 >
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
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 >
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 >
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 >
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 >
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
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
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
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
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(¶ms, optval, optlen)) { + retval = -EFAULT; + goto out; + } + + asoc = sctp_id2assoc(sk, params.sas_assoc_id); + if (!asoc) + goto out; + + retval = sctp_send_add_streams(asoc, ¶ms); + +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; +}
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(+)