RE: sctp: num_ostreams and max_instreams negotiation

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

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

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

PiA+IEF0IHNvbWUgcG9pbnQgdGhlIG5lZ290aWF0aW9uIG9mIHRoZSBudW1iZXIgb2YgU0NUUCBz
dHJlYW1zDQo+ID4gc2VlbXMgdG8gaGF2ZSBnb3QgYnJva2VuLg0KPiA+IEkndmUgZGVmaW5pdGVs
eSB0ZXN0ZWQgaXQgaW4gdGhlIHBhc3QgKHByb2JhYmx5IDEwIHllYXJzIGFnbyEpDQo+ID4gYnV0
IG9uIGEgNS44LjAga2VybmVsIGdldHNvY2tvcHQoU0NUUF9JTkZPKSBzZWVtcyB0byBiZQ0KPiA+
IHJldHVybmluZyB0aGUgJ251bV9vc3RyZWFtcycgc2V0IGJ5IHNldHNvY2tvcHQoU0NUUF9JTklU
KQ0KPiA+IHJhdGhlciB0aGFuIHRoZSBzbWFsbGVyIG9mIHRoYXQgdmFsdWUgYW5kIHRoYXQgY29u
ZmlndXJlZA0KPiA+IGF0IHRoZSBvdGhlciBlbmQgb2YgdGhlIGNvbm5lY3Rpb24uDQo+ID4NCj4g
PiBJJ2xsIGRvIGEgYml0IG9mIGRpZ2dpbmcuDQo+IA0KPiBJIGNhbid0IGZpbmQgdGhlIGNvZGUg
dGhhdCBwcm9jZXNzZXMgdGhlIGluaXRfYWNrLg0KPiBCdXQgd2hlbiBzY3RwX3Byb2Nzc19pbnQo
KSBzYXZlcyB0aGUgc21hbGxlciB2YWx1ZQ0KPiBpbiBhc29jLT5jLnNpbmludF9tYXhfb3N0cmVh
bXMuDQo+IA0KPiBCdXQgYWZlODk5OTYyZWUwNzkgKGlmIEkndmUgdHlwZWQgaXQgcmlnaHQpIGNo
YW5nZWQNCj4gdGhlIHZhbHVlcyBTQ1RQX0lORk8gcmVwb3J0ZWQuDQo+IEFwcGFyYW50bHkgYWRk
aW5nICdzY3RwIHJlY29uZmlnJyBoYWQgY2hhbmdlZCB0aGluZ3MuDQo+IA0KPiBTbyBJIHN1c3Bl
Y3QgdGhpcyBoYXMgYWxsIGJlZW4gYnJva2VuIGZvciBvdmVyIDMgeWVhcnMuDQoNCkl0IGxvb2tz
IGxpa2UgdGhlIGNoYW5nZXMgdGhhdCBicm9rZSBpdCB3ZW50IGludG8gNC4xMS4NCkkndmUganVz
dCBjaGVja2VkIGEgMy44IGtlcm5lbCBhbmQgdGhhdCBuZWdvdGlhdGVzIHRoZQ0KdmFsdWVzIGRv
d24gaW4gYm90aCBkaXJlY3Rpb25zLg0KDQpJIGRvbid0IGhhdmUgYW55IGtlcm5lbHMgbHVya2lu
ZyBiZXR3ZWVuIDMuOCBhbmQgNC4xNS4NCihZZXMsIEkgY291bGQgYnVpbGQgb25lLCBidXQgaXQg
ZG9lc24ndCByZWFsbHkgaGVscC4pDQoNCglEYXZpZA0KDQotDQpSZWdpc3RlcmVkIEFkZHJlc3Mg
TGFrZXNpZGUsIEJyYW1sZXkgUm9hZCwgTW91bnQgRmFybSwgTWlsdG9uIEtleW5lcywgTUsxIDFQ
VCwgVUsNClJlZ2lzdHJhdGlvbiBObzogMTM5NzM4NiAoV2FsZXMpDQo

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

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

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

	stream->outcnt = outcnt;
	return 0;
}

sctp_stream_alloc_in() is the same.

This is called to reduce the number of streams.
But in that case it does nothing at all.

Which means that the 'convert to genradix' change broke it.
Tag 2075e50caf5ea.

I don't know what 'genradix' arrays or the earlier 'flex_array'
actually look like.
But if 'genradix' is some kind of radix-tree it is probably the
wrong beast for SCTP streams.
Lots of code loops through all of them.

This does mean that it has only been broken since the 5.1
merge window.

While just assigning to stream->outcnt when the value
is reduced will fix the negotiation, I've no idea
what side-effects that has.

	David


	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

PiA+ID4gQXQgc29tZSBwb2ludCB0aGUgbmVnb3RpYXRpb24gb2YgdGhlIG51bWJlciBvZiBTQ1RQ
IHN0cmVhbXMNCj4gPiA+IHNlZW1zIHRvIGhhdmUgZ290IGJyb2tlbi4NCj4gPiA+IEkndmUgZGVm
aW5pdGVseSB0ZXN0ZWQgaXQgaW4gdGhlIHBhc3QgKHByb2JhYmx5IDEwIHllYXJzIGFnbyEpDQo+
ID4gPiBidXQgb24gYSA1LjguMCBrZXJuZWwgZ2V0c29ja29wdChTQ1RQX0lORk8pIHNlZW1zIHRv
IGJlDQo+ID4gPiByZXR1cm5pbmcgdGhlICdudW1fb3N0cmVhbXMnIHNldCBieSBzZXRzb2Nrb3B0
KFNDVFBfSU5JVCkNCj4gPiA+IHJhdGhlciB0aGFuIHRoZSBzbWFsbGVyIG9mIHRoYXQgdmFsdWUg
YW5kIHRoYXQgY29uZmlndXJlZA0KPiA+ID4gYXQgdGhlIG90aGVyIGVuZCBvZiB0aGUgY29ubmVj
dGlvbi4NCj4gPiA+DQo+ID4gPiBJJ2xsIGRvIGEgYml0IG9mIGRpZ2dpbmcuDQo+ID4NCj4gPiBJ
IGNhbid0IGZpbmQgdGhlIGNvZGUgdGhhdCBwcm9jZXNzZXMgdGhlIGluaXRfYWNrLg0KPiA+IEJ1
dCB3aGVuIHNjdHBfcHJvY3NzX2ludCgpIHNhdmVzIHRoZSBzbWFsbGVyIHZhbHVlDQo+ID4gaW4g
YXNvYy0+Yy5zaW5pbnRfbWF4X29zdHJlYW1zLg0KPiA+DQo+ID4gQnV0IGFmZTg5OTk2MmVlMDc5
IChpZiBJJ3ZlIHR5cGVkIGl0IHJpZ2h0KSBjaGFuZ2VkDQo+ID4gdGhlIHZhbHVlcyBTQ1RQX0lO
Rk8gcmVwb3J0ZWQuDQo+ID4gQXBwYXJhbnRseSBhZGRpbmcgJ3NjdHAgcmVjb25maWcnIGhhZCBj
aGFuZ2VkIHRoaW5ncy4NCj4gPg0KPiA+IFNvIEkgc3VzcGVjdCB0aGlzIGhhcyBhbGwgYmVlbiBi
cm9rZW4gZm9yIG92ZXIgMyB5ZWFycy4NCj4gDQo+IEl0IGxvb2tzIGxpa2UgdGhlIGNoYW5nZXMg
dGhhdCBicm9rZSBpdCB3ZW50IGludG8gNC4xMS4NCj4gSSd2ZSBqdXN0IGNoZWNrZWQgYSAzLjgg
a2VybmVsIGFuZCB0aGF0IG5lZ290aWF0ZXMgdGhlDQo+IHZhbHVlcyBkb3duIGluIGJvdGggZGly
ZWN0aW9ucy4NCj4gDQo+IEkgZG9uJ3QgaGF2ZSBhbnkga2VybmVscyBsdXJraW5nIGJldHdlZW4g
My44IGFuZCA0LjE1Lg0KPiAoWWVzLCBJIGNvdWxkIGJ1aWxkIG9uZSwgYnV0IGl0IGRvZXNuJ3Qg
cmVhbGx5IGhlbHAuKQ0KDQpPaywgYnVnIGxvY2F0ZWQgLSBwcmV0dHkgb2J2aW91cyByZWFsbHku
DQpuZXQvc2N0cC9zdHJlYW0uIGhhcyB0aGUgZm9sbG93aW5nIGNvZGU6DQoNCnN0YXRpYyBpbnQg
c2N0cF9zdHJlYW1fYWxsb2Nfb3V0KHN0cnVjdCBzY3RwX3N0cmVhbSAqc3RyZWFtLCBfX3UxNiBv
dXRjbnQsDQoJCQkJIGdmcF90IGdmcCkNCnsNCglpbnQgcmV0Ow0KDQoJaWYgKG91dGNudCA8PSBz
dHJlYW0tPm91dGNudCkNCgkJcmV0dXJuIDA7DQoNCglyZXQgPSBnZW5yYWRpeF9wcmVhbGxvYygm
c3RyZWFtLT5vdXQsIG91dGNudCwgZ2ZwKTsNCglpZiAocmV0KQ0KCQlyZXR1cm4gcmV0Ow0KDQoJ
c3RyZWFtLT5vdXRjbnQgPSBvdXRjbnQ7DQoJcmV0dXJuIDA7DQp9DQoNCnNjdHBfc3RyZWFtX2Fs
bG9jX2luKCkgaXMgdGhlIHNhbWUuDQoNClRoaXMgaXMgY2FsbGVkIHRvIHJlZHVjZSB0aGUgbnVt
YmVyIG9mIHN0cmVhbXMuDQpCdXQgaW4gdGhhdCBjYXNlIGl0IGRvZXMgbm90aGluZyBhdCBhbGwu
DQoNCldoaWNoIG1lYW5zIHRoYXQgdGhlICdjb252ZXJ0IHRvIGdlbnJhZGl4JyBjaGFuZ2UgYnJv
a2UgaXQuDQpUYWcgMjA3NWU1MGNhZjVlYS4NCg0KSSBkb24ndCBrbm93IHdoYXQgJ2dlbnJhZGl4
JyBhcnJheXMgb3IgdGhlIGVhcmxpZXIgJ2ZsZXhfYXJyYXknDQphY3R1YWxseSBsb29rIGxpa2Uu
DQpCdXQgaWYgJ2dlbnJhZGl4JyBpcyBzb21lIGtpbmQgb2YgcmFkaXgtdHJlZSBpdCBpcyBwcm9i
YWJseSB0aGUNCndyb25nIGJlYXN0IGZvciBTQ1RQIHN0cmVhbXMuDQpMb3RzIG9mIGNvZGUgbG9v
cHMgdGhyb3VnaCBhbGwgb2YgdGhlbS4NCg0KVGhpcyBkb2VzIG1lYW4gdGhhdCBpdCBoYXMgb25s
eSBiZWVuIGJyb2tlbiBzaW5jZSB0aGUgNS4xDQptZXJnZSB3aW5kb3cuDQoNCldoaWxlIGp1c3Qg
YXNzaWduaW5nIHRvIHN0cmVhbS0+b3V0Y250IHdoZW4gdGhlIHZhbHVlDQppcyByZWR1Y2VkIHdp
bGwgZml4IHRoZSBuZWdvdGlhdGlvbiwgSSd2ZSBubyBpZGVhDQp3aGF0IHNpZGUtZWZmZWN0cyB0
aGF0IGhhcy4NCg0KCURhdmlkDQoNCg0KCURhdmlkDQoNCi0NClJlZ2lzdGVyZWQgQWRkcmVzcyBM
YWtlc2lkZSwgQnJhbWxleSBSb2FkLCBNb3VudCBGYXJtLCBNaWx0b24gS2V5bmVzLCBNSzEgMVBU
LCBVSw0KUmVnaXN0cmF0aW9uIE5vOiAxMzk3Mzg2IChXYWxlcykNCg=

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

From: David Laight
> Sent: 14 August 2020 17:18
> 
> > > > 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().


> This does mean that it has only been broken since the 5.1
> merge window.

And is a good candidate for the back-ports.

> 	ret = genradix_prealloc(&stream->out, outcnt, gfp);
> 	if (ret)
> 		return ret;
> 
> 	stream->outcnt = outcnt;
> 	return 0;
> }
> 
> sctp_stream_alloc_in() is the same.
> 
> This is called to reduce the number of streams.
> But in that case it does nothing at all.
> 
> Which means that the 'convert to genradix' change broke it.
> Tag 2075e50caf5ea.
> 
> I don't know what 'genradix' arrays or the earlier 'flex_array'
> actually look like.
> But if 'genradix' is some kind of radix-tree it is probably the
> wrong beast for SCTP streams.
> Lots of code loops through all of them.

Yep, I'm pretty sure a kvmalloc() would be best.

> While just assigning to stream->outcnt when the value
> is reduced will fix the negotiation, I've no idea
> what side-effects that has.

I've done some checks.
The arrays are allocated when an INIT is sent and also before
a received INIT is processed.
So if one side (eg the responder) allocates a very big value
then the associated memory is never freed when the value
is negotiated down.
There is a comment to the effect that this is desirable.

If my quick calculations are correct then each 'in' is 20 bytes
and each 'out' 24 (with a lot of pad bytes).
So the max sizes are 322 and 386 4k pages.

I haven't looked at how many of the 'out' streams gets the
extra, separately allocated, structure.
I suspect the memory footprint for a single SCTP connection
is potentially huge.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

RnJvbTogRGF2aWQgTGFpZ2h0DQo+IFNlbnQ6IDE0IEF1Z3VzdCAyMDIwIDE3OjE4DQo+IA0KPiA+
ID4gPiBBdCBzb21lIHBvaW50IHRoZSBuZWdvdGlhdGlvbiBvZiB0aGUgbnVtYmVyIG9mIFNDVFAg
c3RyZWFtcw0KPiA+ID4gPiBzZWVtcyB0byBoYXZlIGdvdCBicm9rZW4uDQo+ID4gPiA+IEkndmUg
ZGVmaW5pdGVseSB0ZXN0ZWQgaXQgaW4gdGhlIHBhc3QgKHByb2JhYmx5IDEwIHllYXJzIGFnbyEp
DQo+ID4gPiA+IGJ1dCBvbiBhIDUuOC4wIGtlcm5lbCBnZXRzb2Nrb3B0KFNDVFBfSU5GTykgc2Vl
bXMgdG8gYmUNCj4gPiA+ID4gcmV0dXJuaW5nIHRoZSAnbnVtX29zdHJlYW1zJyBzZXQgYnkgc2V0
c29ja29wdChTQ1RQX0lOSVQpDQo+ID4gPiA+IHJhdGhlciB0aGFuIHRoZSBzbWFsbGVyIG9mIHRo
YXQgdmFsdWUgYW5kIHRoYXQgY29uZmlndXJlZA0KPiA+ID4gPiBhdCB0aGUgb3RoZXIgZW5kIG9m
IHRoZSBjb25uZWN0aW9uLg0KPiA+ID4gPg0KPiA+ID4gPiBJJ2xsIGRvIGEgYml0IG9mIGRpZ2dp
bmcuDQo+ID4gPg0KPiA+ID4gSSBjYW4ndCBmaW5kIHRoZSBjb2RlIHRoYXQgcHJvY2Vzc2VzIHRo
ZSBpbml0X2Fjay4NCj4gPiA+IEJ1dCB3aGVuIHNjdHBfcHJvY3NzX2ludCgpIHNhdmVzIHRoZSBz
bWFsbGVyIHZhbHVlDQo+ID4gPiBpbiBhc29jLT5jLnNpbmludF9tYXhfb3N0cmVhbXMuDQo+ID4g
Pg0KPiA+ID4gQnV0IGFmZTg5OTk2MmVlMDc5IChpZiBJJ3ZlIHR5cGVkIGl0IHJpZ2h0KSBjaGFu
Z2VkDQo+ID4gPiB0aGUgdmFsdWVzIFNDVFBfSU5GTyByZXBvcnRlZC4NCj4gPiA+IEFwcGFyYW50
bHkgYWRkaW5nICdzY3RwIHJlY29uZmlnJyBoYWQgY2hhbmdlZCB0aGluZ3MuDQo+ID4gPg0KPiA+
ID4gU28gSSBzdXNwZWN0IHRoaXMgaGFzIGFsbCBiZWVuIGJyb2tlbiBmb3Igb3ZlciAzIHllYXJz
Lg0KPiA+DQo+ID4gSXQgbG9va3MgbGlrZSB0aGUgY2hhbmdlcyB0aGF0IGJyb2tlIGl0IHdlbnQg
aW50byA0LjExLg0KPiA+IEkndmUganVzdCBjaGVja2VkIGEgMy44IGtlcm5lbCBhbmQgdGhhdCBu
ZWdvdGlhdGVzIHRoZQ0KPiA+IHZhbHVlcyBkb3duIGluIGJvdGggZGlyZWN0aW9ucy4NCj4gPg0K
PiA+IEkgZG9uJ3QgaGF2ZSBhbnkga2VybmVscyBsdXJraW5nIGJldHdlZW4gMy44IGFuZCA0LjE1
Lg0KPiA+IChZZXMsIEkgY291bGQgYnVpbGQgb25lLCBidXQgaXQgZG9lc24ndCByZWFsbHkgaGVs
cC4pDQo+IA0KPiBPaywgYnVnIGxvY2F0ZWQgLSBwcmV0dHkgb2J2aW91cyByZWFsbHkuDQo+IG5l
dC9zY3RwL3N0cmVhbS4gaGFzIHRoZSBmb2xsb3dpbmcgY29kZToNCj4gDQo+IHN0YXRpYyBpbnQg
c2N0cF9zdHJlYW1fYWxsb2Nfb3V0KHN0cnVjdCBzY3RwX3N0cmVhbSAqc3RyZWFtLCBfX3UxNiBv
dXRjbnQsDQo+IAkJCQkgZ2ZwX3QgZ2ZwKQ0KPiB7DQo+IAlpbnQgcmV0Ow0KPiANCj4gCWlmIChv
dXRjbnQgPD0gc3RyZWFtLT5vdXRjbnQpDQo+IAkJcmV0dXJuIDA7DQoNCkRlbGV0aW5nIHRoaXMg
Y2hlY2sgaXMgc3VmZmljaWVudCB0byBmaXggdGhlIGNvZGUuDQpBbG9uZyB3aXRoIHRoZSBlcXVp
dmFsZW50IGNoZWNrIGluIHNjdHBfc3RyZWFtLWFsbG9jX2luKCkuDQoNCg0KPiBUaGlzIGRvZXMg
bWVhbiB0aGF0IGl0IGhhcyBvbmx5IGJlZW4gYnJva2VuIHNpbmNlIHRoZSA1LjENCj4gbWVyZ2Ug
d2luZG93Lg0KDQpBbmQgaXMgYSBnb29kIGNhbmRpZGF0ZSBmb3IgdGhlIGJhY2stcG9ydHMuDQoN
Cj4gCXJldCA9IGdlbnJhZGl4X3ByZWFsbG9jKCZzdHJlYW0tPm91dCwgb3V0Y250LCBnZnApOw0K
PiAJaWYgKHJldCkNCj4gCQlyZXR1cm4gcmV0Ow0KPiANCj4gCXN0cmVhbS0+b3V0Y250ID0gb3V0
Y250Ow0KPiAJcmV0dXJuIDA7DQo+IH0NCj4gDQo+IHNjdHBfc3RyZWFtX2FsbG9jX2luKCkgaXMg
dGhlIHNhbWUuDQo+IA0KPiBUaGlzIGlzIGNhbGxlZCB0byByZWR1Y2UgdGhlIG51bWJlciBvZiBz
dHJlYW1zLg0KPiBCdXQgaW4gdGhhdCBjYXNlIGl0IGRvZXMgbm90aGluZyBhdCBhbGwuDQo+IA0K
PiBXaGljaCBtZWFucyB0aGF0IHRoZSAnY29udmVydCB0byBnZW5yYWRpeCcgY2hhbmdlIGJyb2tl
IGl0Lg0KPiBUYWcgMjA3NWU1MGNhZjVlYS4NCj4gDQo+IEkgZG9uJ3Qga25vdyB3aGF0ICdnZW5y
YWRpeCcgYXJyYXlzIG9yIHRoZSBlYXJsaWVyICdmbGV4X2FycmF5Jw0KPiBhY3R1YWxseSBsb29r
IGxpa2UuDQo+IEJ1dCBpZiAnZ2VucmFkaXgnIGlzIHNvbWUga2luZCBvZiByYWRpeC10cmVlIGl0
IGlzIHByb2JhYmx5IHRoZQ0KPiB3cm9uZyBiZWFzdCBmb3IgU0NUUCBzdHJlYW1zLg0KPiBMb3Rz
IG9mIGNvZGUgbG9vcHMgdGhyb3VnaCBhbGwgb2YgdGhlbS4NCg0KWWVwLCBJJ20gcHJldHR5IHN1
cmUgYSBrdm1hbGxvYygpIHdvdWxkIGJlIGJlc3QuDQoNCj4gV2hpbGUganVzdCBhc3NpZ25pbmcg
dG8gc3RyZWFtLT5vdXRjbnQgd2hlbiB0aGUgdmFsdWUNCj4gaXMgcmVkdWNlZCB3aWxsIGZpeCB0
aGUgbmVnb3RpYXRpb24sIEkndmUgbm8gaWRlYQ0KPiB3aGF0IHNpZGUtZWZmZWN0cyB0aGF0IGhh
cy4NCg0KSSd2ZSBkb25lIHNvbWUgY2hlY2tzLg0KVGhlIGFycmF5cyBhcmUgYWxsb2NhdGVkIHdo
ZW4gYW4gSU5JVCBpcyBzZW50IGFuZCBhbHNvIGJlZm9yZQ0KYSByZWNlaXZlZCBJTklUIGlzIHBy
b2Nlc3NlZC4NClNvIGlmIG9uZSBzaWRlIChlZyB0aGUgcmVzcG9uZGVyKSBhbGxvY2F0ZXMgYSB2
ZXJ5IGJpZyB2YWx1ZQ0KdGhlbiB0aGUgYXNzb2NpYXRlZCBtZW1vcnkgaXMgbmV2ZXIgZnJlZWQg
d2hlbiB0aGUgdmFsdWUNCmlzIG5lZ290aWF0ZWQgZG93bi4NClRoZXJlIGlzIGEgY29tbWVudCB0
byB0aGUgZWZmZWN0IHRoYXQgdGhpcyBpcyBkZXNpcmFibGUuDQoNCklmIG15IHF1aWNrIGNhbGN1
bGF0aW9ucyBhcmUgY29ycmVjdCB0aGVuIGVhY2ggJ2luJyBpcyAyMCBieXRlcw0KYW5kIGVhY2gg
J291dCcgMjQgKHdpdGggYSBsb3Qgb2YgcGFkIGJ5dGVzKS4NClNvIHRoZSBtYXggc2l6ZXMgYXJl
IDMyMiBhbmQgMzg2IDRrIHBhZ2VzLg0KDQpJIGhhdmVuJ3QgbG9va2VkIGF0IGhvdyBtYW55IG9m
IHRoZSAnb3V0JyBzdHJlYW1zIGdldHMgdGhlDQpleHRyYSwgc2VwYXJhdGVseSBhbGxvY2F0ZWQs
IHN0cnVjdHVyZS4NCkkgc3VzcGVjdCB0aGUgbWVtb3J5IGZvb3RwcmludCBmb3IgYSBzaW5nbGUg
U0NUUCBjb25uZWN0aW9uDQppcyBwb3RlbnRpYWxseSBodWdlLg0KDQoJRGF2aWQNCg0KLQ0KUmVn
aXN0ZXJlZCBBZGRyZXNzIExha2VzaWRlLCBCcmFtbGV5IFJvYWQsIE1vdW50IEZhcm0sIE1pbHRv
biBLZXluZXMsIE1LMSAxUFQsIFVLDQpSZWdpc3RyYXRpb24gTm86IDEzOTczODYgKFdhbGVzKQ0K

Re: sctp: num_ostreams and max_instreams negotiation

[Marcelo Ricardo Leitner]

On Sat, Aug 15, 2020 at 02:49:31PM +0000, David Laight wrote:
> From: David Laight
> > Sent: 14 August 2020 17:18
> > 
> > > > > 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?

> 
> 
> > This does mean that it has only been broken since the 5.1
> > merge window.
> 
> And is a good candidate for the back-ports.

Yep.

> 
> > 	ret = genradix_prealloc(&stream->out, outcnt, gfp);
> > 	if (ret)
> > 		return ret;
> > 
> > 	stream->outcnt = outcnt;
> > 	return 0;
> > }
> > 
> > sctp_stream_alloc_in() is the same.
> > 
> > This is called to reduce the number of streams.
> > But in that case it does nothing at all.
> > 
> > Which means that the 'convert to genradix' change broke it.
> > Tag 2075e50caf5ea.
> > 
> > I don't know what 'genradix' arrays or the earlier 'flex_array'
> > actually look like.
> > But if 'genradix' is some kind of radix-tree it is probably the
> > wrong beast for SCTP streams.
> > Lots of code loops through all of them.
> 
> Yep, I'm pretty sure a kvmalloc() would be best.

kvmalloc() doesn't help here because these functions can be called
form bh. Note how sctp_process_strreset_addstrm_in(), for example,
needs to use GFP_ATOMIC in there, in which kvmalloc() can't fallback
to vmalloc.

> 
> > While just assigning to stream->outcnt when the value
> > is reduced will fix the negotiation, I've no idea
> > what side-effects that has.
> 
> I've done some checks.
> The arrays are allocated when an INIT is sent and also before
> a received INIT is processed.
> So if one side (eg the responder) allocates a very big value
> then the associated memory is never freed when the value
> is negotiated down.
> There is a comment to the effect that this is desirable.
> 
> If my quick calculations are correct then each 'in' is 20 bytes
> and each 'out' 24 (with a lot of pad bytes).
> So the max sizes are 322 and 386 4k pages.
> 
> I haven't looked at how many of the 'out' streams gets the
> extra, separately allocated, structure.
> I suspect the memory footprint for a single SCTP connection
> is potentially huge.

This shouldn't be an issue. The default amount of out streams is low
(10, SCTP_DEFAULT_OUTSTREAMS) and the 'in' ones are only allocated
when we have such info already. That's why sctp_connect_new_asoc() and
sctp_association_init() will pass incnt=0 for sctp_stream_init().

  Marcelo

Re: sctp: num_ostreams and max_instreams negotiation

[Marcelo Ricardo Leitner]

On Sat, Aug 15, 2020 at 02:49:31PM +0000, David Laight wrote:
> From: David Laight
> > Sent: 14 August 2020 17:18
> > 
> > > > > 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?

> 
> 
> > This does mean that it has only been broken since the 5.1
> > merge window.
> 
> And is a good candidate for the back-ports.

Yep.

> 
> > 	ret = genradix_prealloc(&stream->out, outcnt, gfp);
> > 	if (ret)
> > 		return ret;
> > 
> > 	stream->outcnt = outcnt;
> > 	return 0;
> > }
> > 
> > sctp_stream_alloc_in() is the same.
> > 
> > This is called to reduce the number of streams.
> > But in that case it does nothing at all.
> > 
> > Which means that the 'convert to genradix' change broke it.
> > Tag 2075e50caf5ea.
> > 
> > I don't know what 'genradix' arrays or the earlier 'flex_array'
> > actually look like.
> > But if 'genradix' is some kind of radix-tree it is probably the
> > wrong beast for SCTP streams.
> > Lots of code loops through all of them.
> 
> Yep, I'm pretty sure a kvmalloc() would be best.

kvmalloc() doesn't help here because these functions can be called
form bh. Note how sctp_process_strreset_addstrm_in(), for example,
needs to use GFP_ATOMIC in there, in which kvmalloc() can't fallback
to vmalloc.

> 
> > While just assigning to stream->outcnt when the value
> > is reduced will fix the negotiation, I've no idea
> > what side-effects that has.
> 
> I've done some checks.
> The arrays are allocated when an INIT is sent and also before
> a received INIT is processed.
> So if one side (eg the responder) allocates a very big value
> then the associated memory is never freed when the value
> is negotiated down.
> There is a comment to the effect that this is desirable.
> 
> If my quick calculations are correct then each 'in' is 20 bytes
> and each 'out' 24 (with a lot of pad bytes).
> So the max sizes are 322 and 386 4k pages.
> 
> I haven't looked at how many of the 'out' streams gets the
> extra, separately allocated, structure.
> I suspect the memory footprint for a single SCTP connection
> is potentially huge.

This shouldn't be an issue. The default amount of out streams is low
(10, SCTP_DEFAULT_OUTSTREAMS) and the 'in' ones are only allocated
when we have such info already. That's why sctp_connect_new_asoc() and
sctp_association_init() will pass incnt=0 for sctp_stream_init().

  Marcelo

Re: sctp: num_ostreams and max_instreams negotiation

[Marcelo Ricardo Leitner]

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:
> > From: David Laight
> > > Sent: 14 August 2020 17:18
> > > 
> > > > > > 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

Re: sctp: num_ostreams and max_instreams negotiation

[Marcelo Ricardo Leitner]

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:
> > From: David Laight
> > > Sent: 14 August 2020 17:18
> > > 
> > > > > > 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

RE: sctp: num_ostreams and max_instreams negotiation

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 17 August 2020 15:36
..
> > 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?

I can, but my systems aren't really setup for doing it.
Especially while working from home.

Just deleting the conditionals works for normal connections.
I don't know what happens if the number of streams is negotiated
up and down (and up again?) while the connection is active.

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

The patch you suggested contained a typo:

-	if (incnt <= stream->incnt)
-		return 0;
+	if (incnt > stream->incnt)
+		goto out;

So the 'in' array was never allocated.

The code will still allocate a 'big' array which I think used
to get shrunk when the value from the peer was processed.
I suspect the array need not get allocated until after
that is done (ISTR in process_init).

I also suspect that the genradix lookup is more expensive
than the sctp code expects.
I wonder if a straight forward kvmalloc() wouldn't be better.
You'd actually need kvrealloc().

All the sctp connections we use have a max of 17 streams.
But if someone allocates 64k and then uses them sparsely
it still allocates about 700 pages for the genradix arrays.

Also, since the 'gfp' flags are being passed in, I suspect
the allocate happens in atomic context somewhere.
I bet allocating 700 pages is very likely to fail!
Lazy allocation would only require single pages be allocated,
but would need extra error paths.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)