On Mon, Nov 26, 2018 at 9:29 PM Marcelo Ricardo Leitner
<marcelo.leitner@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> > ---
> > 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
> >
