netdev - RE: sctp: num_ostreams and max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 18 Aug 2020 08:08:24 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Marcelo Ricardo Leitner' <marcelo.leitner@...il.com>
CC:     "'linux-sctp@...r.kernel.org'" <linux-sctp@...r.kernel.org>,
        'Neil Horman' <nhorman@...driver.com>,
        "'kent.overstreet@...il.com'" <kent.overstreet@...il.com>,
        'Andrew Morton' <akpm@...ux-foundation.org>,
        "'netdev@...r.kernel.org'" <netdev@...r.kernel.org>
Subject: RE: sctp: num_ostreams and max_instreams negotiation

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)