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

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

[Xin Long]

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);
 }
-- 
2.1.0

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

[Marcelo Ricardo Leitner]

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.

>  }
> -- 
> 2.1.0
> 

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

[Xin Long]

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
> >

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

[Marcelo Ricardo Leitner]

On Mon, Nov 26, 2018 at 10:16:50PM +0900, Xin Long wrote:
> 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;".

Good point, sctp_datachk_len needs stream->si there.

> 
> 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;
> 

Can we move the asoc->pathmtu initialization down here too then?
I don't see anything that would block it.
Otherwise LGTM.

> +       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
> > >