| Message ID | 1718155e984f63995d1be9b053c58aa1a645718e.1473326067.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Sep 08, 2016 at 05:31:45PM +0800, Xin Long wrote: > Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks > the asoc's state through statetable before calling sctp_outq_tail. So > there's no need to do it again in sctp_outq_tail. > > This patch is to remove it from sctp_outq_tail. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> This doesn't seem safe to me. The send operation is handled in side effect processing off a queue that might have operations queued ahead of it, which affect the associations state. I think you need to keep this check in place Neil > --- > net/sctp/outqueue.c | 53 ++++++++++++++--------------------------------------- > 1 file changed, 14 insertions(+), 39 deletions(-) > > diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c > index 72e54a4..da2418b 100644 > --- a/net/sctp/outqueue.c > +++ b/net/sctp/outqueue.c > @@ -299,50 +299,25 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp) > * immediately. > */ > if (sctp_chunk_is_data(chunk)) { > - /* Is it OK to queue data chunks? */ > - /* From 9. Termination of Association > - * > - * When either endpoint performs a shutdown, the > - * association on each peer will stop accepting new > - * data from its user and only deliver data in queue > - * at the time of sending or receiving the SHUTDOWN > - * chunk. > - */ > - switch (q->asoc->state) { > - case SCTP_STATE_CLOSED: > - case SCTP_STATE_SHUTDOWN_PENDING: > - case SCTP_STATE_SHUTDOWN_SENT: > - case SCTP_STATE_SHUTDOWN_RECEIVED: > - case SCTP_STATE_SHUTDOWN_ACK_SENT: > - /* Cannot send after transport endpoint shutdown */ > - error = -ESHUTDOWN; > - break; > - > - default: > - pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n", > - __func__, q, chunk, chunk && chunk->chunk_hdr ? > - sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) : > - "illegal chunk"); > - > - sctp_chunk_hold(chunk); > - sctp_outq_tail_data(q, chunk); > - if (chunk->asoc->prsctp_enable && > - SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags)) > - chunk->asoc->sent_cnt_removable++; > - if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED) > - SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS); > - else > - SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS); > - break; > - } > + pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n", > + __func__, q, chunk, chunk && chunk->chunk_hdr ? > + sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) : > + "illegal chunk"); > + > + sctp_chunk_hold(chunk); > + sctp_outq_tail_data(q, chunk); > + if (chunk->asoc->prsctp_enable && > + SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags)) > + chunk->asoc->sent_cnt_removable++; > + if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED) > + SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS); > + else > + SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS); > } else { > list_add_tail(&chunk->list, &q->control_chunk_list); > SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS); > } > > - if (error < 0) > - return error; > - > if (!q->cork) > error = sctp_outq_flush(q, 0, gfp); > > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
>> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks >> the asoc's state through statetable before calling sctp_outq_tail. So >> there's no need to do it again in sctp_outq_tail. >> >> This patch is to remove it from sctp_outq_tail. >> >> Signed-off-by: Xin Long <lucien.xin@gmail.com> > This doesn't seem safe to me. The send operation is handled in side effect > processing off a queue that might have operations queued ahead of it, which > affect the associations state. I think you need to keep this check in place > But not really for Data Chunk. There are 3 places calling sctp_outq_tail. 1. in sctp_assoc_rwnd_increase(): it's for sending sack chunk, so this patch is safe for here 2. sctp_cmd_interpreter(), SCTP_CMD_REPLY: still not Data Chunk, so safe. 3.SCTP_CMD_SEND_MSG: -> sctp_cmd_send_msg() it's for Data Chunk, but it's only called by sctp_sf_do_prm_send(). #define TYPE_SCTP_PRIMITIVE_SEND { \ ... /* SCTP_STATE_COOKIE_WAIT */ \ TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ /* SCTP_STATE_COOKIE_ECHOED */ \ TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ /* SCTP_STATE_ESTABLISHED */ \ TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ /* SCTP_STATE_SHUTDOWN_PENDING */ \ in sctp_sf_do_prm_send(): sctp_add_cmd_sf(commands, SCTP_CMD_SEND_MSG, SCTP_DATAMSG(msg)); return SCTP_DISPOSITION_CONSUME; This function is called by PRIMITIVE_SEND, and every time after calling sctp_do_sm(), there must be no operations inside. so it's impossible that other operations queued ahead of it. I'm not sure if I miss some special case, pls correct me if I'm wrong. Thanks.
On Fri, Sep 09, 2016 at 01:34:05AM +0800, Xin Long wrote: > >> Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks > >> the asoc's state through statetable before calling sctp_outq_tail. So > >> there's no need to do it again in sctp_outq_tail. > >> > >> This patch is to remove it from sctp_outq_tail. > >> > >> Signed-off-by: Xin Long <lucien.xin@gmail.com> > > This doesn't seem safe to me. The send operation is handled in side effect > > processing off a queue that might have operations queued ahead of it, which > > affect the associations state. I think you need to keep this check in place > > > But not really for Data Chunk. > > There are 3 places calling sctp_outq_tail. > 1. in sctp_assoc_rwnd_increase(): > it's for sending sack chunk, so this patch is safe for here > 2. sctp_cmd_interpreter(), SCTP_CMD_REPLY: > still not Data Chunk, so safe. > 3.SCTP_CMD_SEND_MSG: -> sctp_cmd_send_msg() > it's for Data Chunk, but it's only called by sctp_sf_do_prm_send(). > Its not enough to just look at the paths where outq_tail is called, because the outq_tail function is checking a state variable that can update asynchronously in the side effect processing queue. Look at any one of the timer functions. Those all fire asynchronous to the outq list, but they all enqueue to the side effect list. Its entirely possible that the state can be ESTABLISHED in the primitive send path, but before you enqueue the SEND sideffect, a timer will pop and enqueue a NEW_STATE side effect. The end result is you need to check the state again. Neil > #define TYPE_SCTP_PRIMITIVE_SEND { \ > ... > /* SCTP_STATE_COOKIE_WAIT */ \ > TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ > /* SCTP_STATE_COOKIE_ECHOED */ \ > TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ > /* SCTP_STATE_ESTABLISHED */ \ > TYPE_SCTP_FUNC(sctp_sf_do_prm_send), \ > /* SCTP_STATE_SHUTDOWN_PENDING */ \ > > > in sctp_sf_do_prm_send(): > sctp_add_cmd_sf(commands, SCTP_CMD_SEND_MSG, SCTP_DATAMSG(msg)); > return SCTP_DISPOSITION_CONSUME; > > This function is called by PRIMITIVE_SEND, and every time after > calling sctp_do_sm(), there must be no operations inside. > so it's impossible that other operations queued ahead of it. > > I'm not sure if I miss some special case, pls correct me if I'm wrong. > > Thanks. > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
>> > Its not enough to just look at the paths where outq_tail is called, because > the outq_tail function is checking a state variable that can update > asynchronously in the side effect processing queue. Look at any one of the > timer functions. Those all fire asynchronous to the outq list, but they all Fortunately, sctp_do_sm is protected by lock_sock. I just checked all the sctp_do_sm calling in timer functions. ------------ bh_lock_sock(sk); if (sock_owned_by_user(sk)) { <----------- [1] pr_debug("%s: sock is busy\n", __func__); /* Try again later. */ mod_timer() goto out_unlock; } error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT, out_unlock: bh_unlock_sock(sk); which means sctp_do_sm is *non-reenterable* because of sock lock's protection. even if it's interrupted by something like timers, they have to delay their real process until sock lock is released. So It's still safe here. pls double-check.
On Fri, Sep 09, 2016 at 03:11:57PM +0800, Xin Long wrote: > >> > > Its not enough to just look at the paths where outq_tail is called, because > > the outq_tail function is checking a state variable that can update > > asynchronously in the side effect processing queue. Look at any one of the > > timer functions. Those all fire asynchronous to the outq list, but they all > > Fortunately, sctp_do_sm is protected by lock_sock. > I just checked all the sctp_do_sm calling in timer functions. > > ------------ > bh_lock_sock(sk); > if (sock_owned_by_user(sk)) { <----------- [1] > pr_debug("%s: sock is busy\n", __func__); > /* Try again later. */ > mod_timer() > goto out_unlock; > } > error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT, > out_unlock: > bh_unlock_sock(sk); > > > which means sctp_do_sm is *non-reenterable* because of > sock lock's protection. even if it's interrupted by something > like timers, they have to delay their real process until sock > lock is released. > > So It's still safe here. pls double-check. > I don't know, I still don't feel safe about it. I agree the socket lock keeps the state from changing during a single transmission, which makes the use case you are focused on correct. That said, have you considered the retransmit case? That is to say, if you queue and flush the outq, and some packets fail delivery, and in the time between the intial send and the expiration of the RTX timer (during which the socket lock will have been released), an event may occur which changes the transport state, which will then be ignored with your patch. Neil
> I don't know, I still don't feel safe about it. I agree the socket lock keeps > the state from changing during a single transmission, which makes the use case > you are focused on correct. ok, :-) > > That said, have you considered the retransmit case? That is to say, if you > queue and flush the outq, and some packets fail delivery, and in the time > between the intial send and the expiration of the RTX timer (during which the > socket lock will have been released), an event may occur which changes the > transport state, which will then be ignored with your patch. Sorry, I'm not sure if I got it. You mean "during which changes q->asoc->state", right ? This patch removes the check of q->asoc->state in sctp_outq_tail(). sctp_outq_tail() is called for data only in: sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg -> sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail() before calling sctp_primitive_SEND, hold sock lock first. then sctp_primitive_SEND choose FUNC according: #define TYPE_SCTP_PRIMITIVE_SEND { .... if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg, but sctp_sf_error_closed/sctp_sf_error_shutdown, sctp_outq_tail can't be called, either. I mean sctp_primitive_SEND do the same check for asoc->state already actually. so the code in sctp_outq_tail is redundant actually. > > Neil >
On Sat, Sep 10, 2016 at 12:03:53AM +0800, Xin Long wrote: > > That said, have you considered the retransmit case? That is to say, if you > > queue and flush the outq, and some packets fail delivery, and in the time > > between the intial send and the expiration of the RTX timer (during which the > > socket lock will have been released), an event may occur which changes the > > transport state, which will then be ignored with your patch. > Sorry, I'm not sure if I got it. > > You mean "during which changes q->asoc->state", right ? > > This patch removes the check of q->asoc->state in sctp_outq_tail(). > > sctp_outq_tail() is called for data only in: > sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg -> > sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail() > > before calling sctp_primitive_SEND, hold sock lock first. > then sctp_primitive_SEND choose FUNC according: > > #define TYPE_SCTP_PRIMITIVE_SEND { > .... > > if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg, > but sctp_sf_error_closed/sctp_sf_error_shutdown, sctp_outq_tail > can't be called, either. > I mean sctp_primitive_SEND do the same check for asoc->state > already actually. > > so the code in sctp_outq_tail is redundant actually. I also don't see an issue with this patch, btw. Xin, you may want to add more/such details to the changelog, specially about the timer versus primitive handling. Marcelo
On Sat, Sep 10, 2016 at 12:03:53AM +0800, Xin Long wrote: > > I don't know, I still don't feel safe about it. I agree the socket lock keeps > > the state from changing during a single transmission, which makes the use case > > you are focused on correct. > ok, :-) > > > > > That said, have you considered the retransmit case? That is to say, if you > > queue and flush the outq, and some packets fail delivery, and in the time > > between the intial send and the expiration of the RTX timer (during which the > > socket lock will have been released), an event may occur which changes the > > transport state, which will then be ignored with your patch. > Sorry, I'm not sure if I got it. > > You mean "during which changes q->asoc->state", right ? > > This patch removes the check of q->asoc->state in sctp_outq_tail(). > > sctp_outq_tail() is called for data only in: > sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg -> > sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail() > > before calling sctp_primitive_SEND, hold sock lock first. > then sctp_primitive_SEND choose FUNC according: > > #define TYPE_SCTP_PRIMITIVE_SEND { > .... > > if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg, > but sctp_sf_error_closed/sctp_sf_error_shutdown, sctp_outq_tail > can't be called, either. > I mean sctp_primitive_SEND do the same check for asoc->state > already actually. > > so the code in sctp_outq_tail is redundant actually. > > > > > > > > Neil > > > Ok, you've convinced me, thanks for taking the time to go through it Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > I also don't see an issue with this patch, btw. > > Xin, you may want to add more/such details to the changelog, specially > about the timer versus primitive handling. > OK, I will post v2 of this patchset.
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index 72e54a4..da2418b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -299,50 +299,25 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk, gfp_t gfp) * immediately. */ if (sctp_chunk_is_data(chunk)) { - /* Is it OK to queue data chunks? */ - /* From 9. Termination of Association - * - * When either endpoint performs a shutdown, the - * association on each peer will stop accepting new - * data from its user and only deliver data in queue - * at the time of sending or receiving the SHUTDOWN - * chunk. - */ - switch (q->asoc->state) { - case SCTP_STATE_CLOSED: - case SCTP_STATE_SHUTDOWN_PENDING: - case SCTP_STATE_SHUTDOWN_SENT: - case SCTP_STATE_SHUTDOWN_RECEIVED: - case SCTP_STATE_SHUTDOWN_ACK_SENT: - /* Cannot send after transport endpoint shutdown */ - error = -ESHUTDOWN; - break; - - default: - pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n", - __func__, q, chunk, chunk && chunk->chunk_hdr ? - sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) : - "illegal chunk"); - - sctp_chunk_hold(chunk); - sctp_outq_tail_data(q, chunk); - if (chunk->asoc->prsctp_enable && - SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags)) - chunk->asoc->sent_cnt_removable++; - if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED) - SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS); - else - SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS); - break; - } + pr_debug("%s: outqueueing: outq:%p, chunk:%p[%s])\n", + __func__, q, chunk, chunk && chunk->chunk_hdr ? + sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) : + "illegal chunk"); + + sctp_chunk_hold(chunk); + sctp_outq_tail_data(q, chunk); + if (chunk->asoc->prsctp_enable && + SCTP_PR_PRIO_ENABLED(chunk->sinfo.sinfo_flags)) + chunk->asoc->sent_cnt_removable++; + if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED) + SCTP_INC_STATS(net, SCTP_MIB_OUTUNORDERCHUNKS); + else + SCTP_INC_STATS(net, SCTP_MIB_OUTORDERCHUNKS); } else { list_add_tail(&chunk->list, &q->control_chunk_list); SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS); } - if (error < 0) - return error; - if (!q->cork) error = sctp_outq_flush(q, 0, gfp);
Data Chunks are only sent by sctp_primitive_SEND, in which sctp checks the asoc's state through statetable before calling sctp_outq_tail. So there's no need to do it again in sctp_outq_tail. This patch is to remove it from sctp_outq_tail. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/outqueue.c | 53 ++++++++++++++--------------------------------------- 1 file changed, 14 insertions(+), 39 deletions(-)