Re: [PATCH net 1/2] sctp: factor out INIT verification failure handling

On Wed, Jun 17, 2026 at 6:40 AM Simon Horman <[email protected]> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> sctp: factor out INIT verification failure handling
>
> This refactor moves the duplicated INIT/INIT-ACK error handling out of
> sctp_sf_do_5_1B_init(), sctp_sf_do_5_1C_ack(), and
> sctp_sf_do_unexpected_init() into a new helper sctp_abort_on_init_err().
> The commit message states "No functional change intended."
>
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index 9b23c11cbb9e..544f308ee527 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
>
> [ ... ]
>
> > @@ -548,50 +531,9 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
> >       err_chunk = NULL;
> >       if (!sctp_verify_init(net, ep, asoc, chunk->chunk_hdr->type,
> >                             (struct sctp_init_chunk *)chunk->chunk_hdr, chunk,
> > -                           &err_chunk)) {
> > -
> > -             enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
> > -
> > -             /* This chunk contains fatal error. It is to be discarded.
> > -              * Send an ABORT, with causes.  If there are no causes,
> > -              * then there wasn't enough memory.  Just terminate
> > -              * the association.
> > -              */
> > -             if (err_chunk) {
> > -                     packet = sctp_abort_pkt_new(net, ep, asoc, arg,
> > -                                     (__u8 *)(err_chunk->chunk_hdr) +
> > -                                     sizeof(struct sctp_chunkhdr),
> > -                                     ntohs(err_chunk->chunk_hdr->length) -
> > -                                     sizeof(struct sctp_chunkhdr));
> > -
> > -                     sctp_chunk_free(err_chunk);
> > -
> > -                     if (packet) {
> > -                             sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
> > -                                             SCTP_PACKET(packet));
> > -                             SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
> > -                             error = SCTP_ERROR_INV_PARAM;
> > -                     }
> > -             }
> > -
> > -             /* SCTP-AUTH, Section 6.3:
> > -              *    It should be noted that if the receiver wants to tear
> > -              *    down an association in an authenticated way only, the
> > -              *    handling of malformed packets should not result in
> > -              *    tearing down the association.
> > -              *
> > -              * This means that if we only want to abort associations
> > -              * in an authenticated way (i.e AUTH+ABORT), then we
> > -              * can't destroy this association just because the packet
> > -              * was malformed.
> > -              */
> > -             if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
> > -                     return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> > -
> > -             SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> > -             return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
> > -                                             asoc, chunk->transport);
> > -     }
> > +                           &err_chunk))
> > +             return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
> > +                                           commands, err_chunk);
>
> [High]
> In the original sctp_sf_do_5_1C_ack() the "if (err_chunk) { ... }"
> block is non-controlling: when err_chunk is NULL the code skips the
> block, falls through with error == SCTP_ERROR_NO_RESOURCE, consults
> sctp_auth_recv_cid(SCTP_CID_ABORT, asoc) and either pdiscards or runs
> sctp_stop_t1_and_abort() to tear the half-open association down.
>
> [High]
> In the new helper, can the INIT-ACK + NULL err_chunk case still reach
> that path?  Looking at sctp_abort_on_init_err():
>
>   if (!err_chunk)
>           return sctp_sf_tabort_8_4_8(net, ep, asoc, SCTP_ST_CHUNK(0),
>                                       arg, commands);
>
>   ...
>   if (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) {
>           ...
>   }
>   /* SCTP-AUTH, Section 6.3 ... */
>   if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
>           return sctp_sf_pdiscard(...);
>   SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
>   return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
>                                 asoc, chunk->transport);
>
> [High]
> The !err_chunk early return runs before the INIT-ACK type check, so
> when sctp_verify_init() returns 0 with *errp left as NULL on an
> INIT-ACK, control now goes to sctp_sf_tabort_8_4_8() rather than the
> auth-check + sctp_stop_t1_and_abort() path.
>
> [High]
> Is the NULL-err_chunk case actually reachable on an INIT-ACK?  In
> sctp_process_inv_mandatory() and sctp_process_inv_paramlength() in
> net/sctp/sm_make_chunk.c, *errp is allocated via sctp_make_op_error_space()
> or sctp_make_violation_paramlen() and the function returns 0
> unconditionally:
>
>   if (!*errp)
>           *errp = sctp_make_op_error_space(asoc, chunk, 0);
>   if (*errp)
>           sctp_init_cause(...);
>   return 0;
>
> So an allocation failure on a malformed INIT-ACK reaches the helper
> with err_chunk == NULL.
>
> [High]
> On that path, does the new code skip several actions that the original
> performed?  sctp_sf_tabort_8_4_8() emits an OOTB ABORT and pdiscards;
> it does not stop T1-init, does not call SCTP_CMD_SET_SK_ERR(ECONNREFUSED),
> does not issue SCTP_CMD_INIT_FAILED, does not transition the asoc to
> SCTP_STATE_CLOSED, and does not increment SCTP_MIB_ABORTEDS.
>
> [High]
> As a result, would connect(2) wait for T1-init to exhaust its
> retransmissions instead of receiving ECONNREFUSED promptly, and would
> SCTP_MIB_ABORTEDS no longer be incremented (with SCTP_MIB_IN_PKT_DISCARDS
> being incremented in its place)?
>
> [High]
> Does this also bypass the SCTP-AUTH section 6.3 check quoted in the
> comment?  Previously, when AUTH-ABORT was negotiated and err_chunk
> was NULL, sctp_auth_recv_cid() caused sctp_sf_pdiscard() to be returned
> and no ABORT was emitted on the wire.  In the new path, sctp_sf_tabort_8_4_8()
> sends an unauthenticated OOTB ABORT regardless of AUTH negotiation.
>
> [High]
> Finally, does the disposition return value also change from
> SCTP_DISPOSITION_ABORT to SCTP_DISPOSITION_CONSUME on this path?
>
> [High]
> Would moving the chunk->chunk_hdr->type check ahead of the !err_chunk
> early return restore the original semantics for INIT-ACK while keeping
> the sctp_sf_tabort_8_4_8() fallback for INIT?
>
All above is talking about one regression, the (!err_chunk) check should be
after (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) check.

This is a good catch, and I will fix it on v2.

Thanks.

> [ ... ]
>
> > @@ -6485,6 +6403,61 @@ static void sctp_send_stale_cookie_err(struct net *net,
> >       }
> >  }
> >
> > +static enum sctp_disposition sctp_abort_on_init_err(
> > +                                     struct net *net,
> > +                                     const struct sctp_endpoint *ep,
> > +                                     const struct sctp_association *asoc,
> > +                                     const struct sctp_chunk *chunk,
> > +                                     void *arg,
> > +                                     struct sctp_cmd_seq *commands,
> > +                                     struct sctp_chunk *err_chunk)
> > +{
> > +     enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
> > +     struct sctp_packet *packet;
> > +     struct sctp_chunkhdr *ch;
> > +
> > +     if (!err_chunk)
> > +             return sctp_sf_tabort_8_4_8(net, ep, asoc, SCTP_ST_CHUNK(0),
> > +                                         arg, commands);
> > +
> > +     ch = err_chunk->chunk_hdr;
> > +     packet = sctp_abort_pkt_new(net, ep, asoc, arg,
> > +                                 (__u8 *)ch + sizeof(*ch),
> > +                                 ntohs(ch->length) - sizeof(*ch));
> > +
> > +     sctp_chunk_free(err_chunk);
> > +
> > +     if (packet) {
> > +             sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
> > +                             SCTP_PACKET(packet));
> > +             SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
> > +             error = SCTP_ERROR_INV_PARAM;
> > +     }
> > +
> > +     if (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) {
> > +             if (!packet)
> > +                     return SCTP_DISPOSITION_NOMEM;
> > +             return SCTP_DISPOSITION_CONSUME;
> > +     }
> > +     /* SCTP-AUTH, Section 6.3:
> > +      *    It should be noted that if the receiver wants to tear
> > +      *    down an association in an authenticated way only, the
> > +      *    handling of malformed packets should not result in
> > +      *    tearing down the association.
> > +      *
> > +      * This means that if we only want to abort associations
> > +      * in an authenticated way (i.e AUTH+ABORT), then we
> > +      * can't destroy this association just because the packet
> > +      * was malformed.
> > +      */
> > +     if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
> > +             return sctp_sf_pdiscard(net, ep, asoc, SCTP_ST_CHUNK(0), arg,
> > +                                     commands);
> > +
> > +     SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> > +     return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
> > +                                   asoc, chunk->transport);
> > +}