| Message ID | cover.1619812899.git.lucien.xin@gmail.com |
|---|---|
| Headers | show |
| Series | sctp: always send a chunk with the asoc that it belongs to | expand |
Hello: This series was applied to netdev/net.git (refs/heads/master): On Sat, 1 May 2021 04:02:57 +0800 you wrote: > Currently when processing a duplicate COOKIE-ECHO chunk, a new temp > asoc would be created, then it creates the chunks with the new asoc. > However, later on it uses the old asoc to send these chunks, which > has caused quite a few issues. > > This patchset is to fix this and make sure that the COOKIE-ACK and > SHUTDOWN chunks are created with the same asoc that will be used to > send them out. > > [...] Here is the summary with links: - [PATCHv2,net,1/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_a https://git.kernel.org/netdev/net/c/35b4f24415c8 - [PATCHv2,net,2/3] Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK" https://git.kernel.org/netdev/net/c/7e9269a5acec - [PATCHv2,net,3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b https://git.kernel.org/netdev/net/c/51eac7f2f06b You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Fri, 30 Apr 2021, Xin Long wrote: > Currently when processing a duplicate COOKIE-ECHO chunk, a new temp > asoc would be created, then it creates the chunks with the new asoc. > However, later on it uses the old asoc to send these chunks, which > has caused quite a few issues. > > This patchset is to fix this and make sure that the COOKIE-ACK and > SHUTDOWN chunks are created with the same asoc that will be used to > send them out. Again, much thanks for looking into this. Patches 1 and 3 are almost the same as my patch, which as I mentioned I've been testing on and off for the past couple of weeks, and haven't found any problems. (Then again, I didn't find any problems last time either.) I think 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with COOKIE-ACK") should not be reverted (I'll reply to the patch). 12dfd78e3a74 ("sctp: Fix SHUTDOWN CTSN Ack in the peer restart case") should be reverted. With association update no longer a side effect, we can get CTSN normally from current assoc, since it has been updated before sctp_make_shutdown(). > > v1->v2: > - see Patch 3/3. > > Xin Long (3): > sctp: do asoc update earlier in sctp_sf_do_dupcook_a > Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK" > sctp: do asoc update earlier in sctp_sf_do_dupcook_b > > include/net/sctp/command.h | 1 - > net/sctp/sm_sideeffect.c | 26 ------------------------ > net/sctp/sm_statefuns.c | 50 ++++++++++++++++++++++++++++++++++++---------- > 3 files changed, 39 insertions(+), 38 deletions(-) > > -- > 2.1.0
On Sat, May 1, 2021 at 12:57 PM Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com> wrote: > > On Fri, 30 Apr 2021, Xin Long wrote: > > > The same thing should be done for sctp_sf_do_dupcook_b(). > > Meanwhile, SCTP_CMD_UPDATE_ASSOC cmd can be removed. > > > > v1->v2: > > - Fix the return value in sctp_sf_do_assoc_update(). > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > --- > > include/net/sctp/command.h | 1 - > > net/sctp/sm_sideeffect.c | 26 ------------------------- > > net/sctp/sm_statefuns.c | 47 +++++++++++++++++++++++++++++----------------- > > 3 files changed, 30 insertions(+), 44 deletions(-) > > > > diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h > > index e8df72e..5e84888 100644 > > --- a/include/net/sctp/command.h > > +++ b/include/net/sctp/command.h > > @@ -68,7 +68,6 @@ enum sctp_verb { > > SCTP_CMD_ASSOC_FAILED, /* Handle association failure. */ > > SCTP_CMD_DISCARD_PACKET, /* Discard the whole packet. */ > > SCTP_CMD_GEN_SHUTDOWN, /* Generate a SHUTDOWN chunk. */ > > - SCTP_CMD_UPDATE_ASSOC, /* Update association information. */ > > SCTP_CMD_PURGE_OUTQUEUE, /* Purge all data waiting to be sent. */ > > SCTP_CMD_SETUP_T2, /* Hi-level, setup T2-shutdown parms. */ > > SCTP_CMD_RTO_PENDING, /* Set transport's rto_pending. */ > > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c > > index 0948f14..ce15d59 100644 > > --- a/net/sctp/sm_sideeffect.c > > +++ b/net/sctp/sm_sideeffect.c > > @@ -826,28 +826,6 @@ static void sctp_cmd_setup_t2(struct sctp_cmd_seq *cmds, > > asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = t->rto; > > } > > > > -static void sctp_cmd_assoc_update(struct sctp_cmd_seq *cmds, > > - struct sctp_association *asoc, > > - struct sctp_association *new) > > -{ > > - struct net *net = asoc->base.net; > > - struct sctp_chunk *abort; > > - > > - if (!sctp_assoc_update(asoc, new)) > > - return; > > - > > - abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr)); > > - if (abort) { > > - sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0); > > - sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); > > - } > > - sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED)); > > - sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED, > > - SCTP_PERR(SCTP_ERROR_RSRC_LOW)); > > - SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS); > > - SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB); > > -} > > - > > /* Helper function to change the state of an association. */ > > static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds, > > struct sctp_association *asoc, > > @@ -1301,10 +1279,6 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type, > > sctp_endpoint_add_asoc(ep, asoc); > > break; > > > > - case SCTP_CMD_UPDATE_ASSOC: > > - sctp_cmd_assoc_update(commands, asoc, cmd->obj.asoc); > > - break; > > - > > case SCTP_CMD_PURGE_OUTQUEUE: > > sctp_outq_teardown(&asoc->outqueue); > > break; > > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c > > index e8ccc4e..a428449 100644 > > --- a/net/sctp/sm_statefuns.c > > +++ b/net/sctp/sm_statefuns.c > > @@ -1773,6 +1773,30 @@ enum sctp_disposition sctp_sf_do_5_2_3_initack( > > return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands); > > } > > > > +static int sctp_sf_do_assoc_update(struct sctp_association *asoc, > > + struct sctp_association *new, > > + struct sctp_cmd_seq *cmds) > > +{ > > + struct net *net = asoc->base.net; > > + struct sctp_chunk *abort; > > + > > + if (!sctp_assoc_update(asoc, new)) > > + return 0; > > + > > + abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr)); > > + if (abort) { > > + sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0); > > + sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); > > + } > > + sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED)); > > + sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED, > > + SCTP_PERR(SCTP_ERROR_RSRC_LOW)); > > + SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS); > > + SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB); > > + > > + return -ENOMEM; > > +} > > + > > /* Unexpected COOKIE-ECHO handler for peer restart (Table 2, action 'A') > > * > > * Section 5.2.4 > > @@ -1853,21 +1877,8 @@ static enum sctp_disposition sctp_sf_do_dupcook_a( > > sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL()); > > > > /* Update the content of current association. */ > > - if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) { > > - struct sctp_chunk *abort; > > - > > - abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr)); > > - if (abort) { > > - sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0); > > - sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort)); > > - } > > - sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED)); > > - sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED, > > - SCTP_PERR(SCTP_ERROR_RSRC_LOW)); > > - SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS); > > - SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB); > > + if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands)) > > goto nomem; > > - } > > > > repl = sctp_make_cookie_ack(asoc, chunk); > > if (!repl) > > @@ -1940,14 +1951,16 @@ static enum sctp_disposition sctp_sf_do_dupcook_b( > > if (!sctp_auth_chunk_verify(net, chunk, new_asoc)) > > return SCTP_DISPOSITION_DISCARD; > > > > - /* Update the content of current association. */ > > - sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc)); > > sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE, > > SCTP_STATE(SCTP_STATE_ESTABLISHED)); > > SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB); > > sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL()); > > > > - repl = sctp_make_cookie_ack(new_asoc, chunk); > > + /* Update the content of current association. */ > > + if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands)) > > + goto nomem; > > Wouldn't it be better to do the update before SCTP_CMD_NEW_STATE? > Or do we have some reason to move to SCTP_STATE_ESTABLISHED even > if the update fails? Yes, as sctp_sf_do_assoc_update() adds SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB), and before that we have to keep SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB) added in there, which is normally right after NEW_STATE cmd with ESTABLISHED. > > > + > > + repl = sctp_make_cookie_ack(asoc, chunk); > > if (!repl) > > goto nomem; > > > > -- > > 2.1.0
On Sat, May 1, 2021 at 12:54 PM Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com> wrote: > > On Fri, 30 Apr 2021, Xin Long wrote: > > > Currently when processing a duplicate COOKIE-ECHO chunk, a new temp > > asoc would be created, then it creates the chunks with the new asoc. > > However, later on it uses the old asoc to send these chunks, which > > has caused quite a few issues. > > > > This patchset is to fix this and make sure that the COOKIE-ACK and > > SHUTDOWN chunks are created with the same asoc that will be used to > > send them out. > > Again, much thanks for looking into this. Patches 1 and 3 are > almost the same as my patch, which as I mentioned I've been > testing on and off for the past couple of weeks, and haven't > found any problems. (Then again, I didn't find any problems last > time either.) > > I think 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with > COOKIE-ACK") should not be reverted (I'll reply to the patch). > > 12dfd78e3a74 ("sctp: Fix SHUTDOWN CTSN Ack in the peer restart > case") should be reverted. With association update no longer a > side effect, we can get CTSN normally from current assoc, since > it has been updated before sctp_make_shutdown(). Good catch, didn't notice this one. Will do it. Thanks. > > > > > v1->v2: > > - see Patch 3/3. > > > > Xin Long (3): > > sctp: do asoc update earlier in sctp_sf_do_dupcook_a > > Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK" > > sctp: do asoc update earlier in sctp_sf_do_dupcook_b > > > > include/net/sctp/command.h | 1 - > > net/sctp/sm_sideeffect.c | 26 ------------------------ > > net/sctp/sm_statefuns.c | 50 ++++++++++++++++++++++++++++++++++++---------- > > 3 files changed, 39 insertions(+), 38 deletions(-) > > > > -- > > 2.1.0