netdev - Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_dzWt_LNu4sT5TtsOEzcg5cJvPqVqphE-QbFDgB7pPJdQ@mail.gmail.com>
Date:   Fri, 25 Oct 2019 15:59:18 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Neil Horman <nhorman@...driver.com>,
        davem <davem@...emloft.net>,
        David Laight <david.laight@...lab.com>
Subject: Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
>
> Hi,
>
> Sorry for the long delay on this review.
>
> On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote:
> > SCTP Quick failover draft section 5.1, point 5 has been removed
> > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > Layer Protocol (ULP) about this state transition", as said in
> > section 3.2, point 8.
> >
> > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > in section 7.1, "which is reported if the affected address
> > becomes PF". Also remove transport cwnd's update when moving
> > from PF back to ACTIVE , which is no longer in rfc7829 either.
> >
> > v1->v2:
> >   - no change
> > v2->v3:
> >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/associola.c      | 17 ++++-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index 6bce7f9..f4ab7bb 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> >       SCTP_ADDR_ADDED,
> >       SCTP_ADDR_MADE_PRIM,
> >       SCTP_ADDR_CONFIRMED,
> > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >  };
> >
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 1ba893b..4f9efba 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>
> While at here, dealing with spc_state, please seize the moment and
> initialize it to the enum instead:
>
> @@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>                                   sctp_sn_error_t error)
>  {
>         bool ulp_notify = true;
> -       int spc_state = 0;
> +       int spc_state = SCTP_ADDR_AVAILABLE;
>
>
> >                       spc_state = SCTP_ADDR_CONFIRMED;
> >               else
> >                       spc_state = SCTP_ADDR_AVAILABLE;
>
> This else could be removed (equals to initial value).
yes, will improve it.

>
> > -             /* Don't inform ULP about transition from PF to
> > -              * active state and set cwnd to 1 MTU, see SCTP
> > -              * Quick failover draft section 5.1, point 5
> > -              */
> > -             if (transport->state == SCTP_PF) {
> > -                     ulp_notify = false;
> > -                     transport->cwnd = asoc->pathmtu;
> > -             }
> >               transport->state = SCTP_ACTIVE;
> >               break;
> >
> > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                * to inactive state.  Also, release the cached route since
> >                * there may be a better route next time.
> >                */
> > -             if (transport->state != SCTP_UNCONFIRMED)
> > +             if (transport->state != SCTP_UNCONFIRMED) {
> >                       transport->state = SCTP_INACTIVE;
> > -             else {
> > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > +             } else {
> >                       sctp_transport_dst_release(transport);
> >                       ulp_notify = false;
> >               }
> > -
> > -             spc_state = SCTP_ADDR_UNREACHABLE;
> >               break;
> >
> >       case SCTP_TRANSPORT_PF:
> >               transport->state = SCTP_PF;
> > -             ulp_notify = false;
> > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >               break;
> >
> >       default:
> > --
> > 2.1.0
> >