Re: [PATCH net] sctp: update frag_point when stream_interleave is set

[Xin Long <lucien.xin@gmail.com>]

On Mon, Nov 26, 2018 at 9:29 PM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Nov 26, 2018 at 05:02:11PM +0800, Xin Long wrote:
> > sctp_assoc_update_frag_point() should be called whenever asoc->pathmtu
> > changes, but we missed one place in sctp_association_init(). It would
> > cause frag_point is zero when sending data.
> >
> > As says in Jakub's reproducer, if sp->pathmtu is set by socketopt, the
> > new asoc->pathmtu inherits it in sctp_association_init(). Later when
> > transports are added and their pmtu >= asoc->pathmtu, it will never
> > call sctp_assoc_update_frag_point() to set frag_point.
> >
> > This patch is to fix it by updating frag_point when stream_interleave
> > is set in sctp_stream_interleave_init(), which is also called in
> > sctp_association_init(). We're doing this also because frag_point
> > is affected by datachunk's type, namely stream_interleave_0/1.
> >
> > Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> > Reported-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/stream_interleave.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> > index 0a78cdf..19d596d 100644
> > --- a/net/sctp/stream_interleave.c
> > +++ b/net/sctp/stream_interleave.c
> > @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct sctp_stream *stream)
> >       asoc = container_of(stream, struct sctp_association, stream);
> >       stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
> >                                      : &sctp_stream_interleave_0;
> > +     sctp_assoc_update_frag_point(asoc);
>
> I get that by adding it here we avoid adding it twice, one in
> sctp_association_init and another in sctp_process_init, but here it is
> out of context.
>
> The decision on data chunk format is not made on this function but
> higher in the stack and we can leverage that for sctp_process_init,
> and for sctp_association_init, we should have it as close as possible
> to where it initialized pathmtu and did not update the frag point.
okay, but both have to be after sctp_stream_init().
though we want sctp_assoc_update_frag_point()
called right after "asoc->pathmtu = sp->pathmtu;".

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index a827a1f..a614937 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -252,6 +252,8 @@ static struct sctp_association *sctp_association_init(
                             0, gfp))
                goto fail_init;

+       sctp_assoc_update_frag_point(asoc);
+
        /* Assume that peer would support both address types unless we are
         * told otherwise.
         */
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 4a4fd19..600ca0d 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2462,6 +2462,8 @@ int sctp_process_init(struct sctp_association
*asoc, struct sctp_chunk *chunk,
                             asoc->c.sinit_max_instreams, gfp))
                goto clean_up;

+       sctp_assoc_update_frag_point(asoc);
+
        if (!asoc->temp && sctp_assoc_set_id(asoc, gfp))
                goto clean_up;

>
> >  }
> > --
> > 2.1.0
> >