From: David Laight <hidden> Date: 2020-08-14 13:36:42
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.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-08-14 16:18:13
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;
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)
From: David Laight <hidden> Date: 2020-08-15 21:57:51
From: David Laight
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().
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)
On Sat, Aug 15, 2020 at 02:49:31PM +0000, David Laight wrote:
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?
quoted
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.
quoted
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.
quoted
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
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?
From: David Laight <hidden> Date: 2020-08-18 08:08:35
From: Marcelo Ricardo Leitner
Sent: 17 August 2020 15:36
..
quoted
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.
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)