On Mon, Aug 17, 2020 at 11:22:23AM -0300, Marcelo Ricardo Leitner wrote:

On Sat, Aug 15, 2020 at 02:49:31PM +0000, David Laight wrote:

quoted

From: David Laight

quoted

Sent: 14 August 2020 17:18

quoted

quoted

quoted

At some point the negotiation of the number of SCTP streams
seems to have got broken.
I've definitely tested it in the past (probably 10 years ago!)
but on a 5.8.0 kernel getsockopt(SCTP_INFO) seems to be
returning the 'num_ostreams' set by setsockopt(SCTP_INIT)
rather than the smaller of that value and that configured
at the other end of the connection.

I'll do a bit of digging.

I can't find the code that processes the init_ack.
But when sctp_procss_int() saves the smaller value
in asoc->c.sinint_max_ostreams.

But afe899962ee079 (if I've typed it right) changed
the values SCTP_INFO reported.
Apparantly adding 'sctp reconfig' had changed things.

So I suspect this has all been broken for over 3 years.

It looks like the changes that broke it went into 4.11.
I've just checked a 3.8 kernel and that negotiates the
values down in both directions.

I don't have any kernels lurking between 3.8 and 4.15.
(Yes, I could build one, but it doesn't really help.)

Ok, bug located - pretty obvious really.
net/sctp/stream. has the following code:

static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
				 gfp_t gfp)
{
	int ret;

	if (outcnt <= stream->outcnt)
		return 0;

Deleting this check is sufficient to fix the code.
Along with the equivalent check in sctp_stream-alloc_in().

2075e50caf5e has:

-       if (outcnt > stream->outcnt)
-               fa_zero(out, stream->outcnt, (outcnt - stream->outcnt));
+       if (outcnt <= stream->outcnt)
+               return 0;

-       stream->out = out;
+       ret = genradix_prealloc(&stream->out, outcnt, gfp);
+       if (ret)
+               return ret;

+       stream->outcnt = outcnt;
        return 0;

The flip on the if() return missed that stream->outcnt needs to be
updated later on even if it is reducing the size.

The proper fix here is to move back to the original if() condition,
and put genradix_prealloc() inside it again, as was fa_zero() before.
The if() is not strictly needed, because genradix_prealloc() will
handle it nicely, but it's a nice-to-have optimization anyway.

Do you want to send a patch?

Note the thread 'Subject: RE: v5.3.12 SCTP Stream Negotiation Problem'
though.

  Marcelo