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
[off-list ref] wrote:

quoted

On Mon, Nov 26, 2018 at 05:02:11PM +0800, Xin Long wrote:

quoted

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

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