From: Marcelo Ricardo Leitner

Sent: 22 May 2020 15:36

On Fri, May 22, 2020 at 08:02:09AM +0000, David Laight wrote:

quoted

From: Christoph Hellwig

quoted

Sent: 21 May 2020 18:47
based on the review of Davids patch to do something similar I dusted off
the series I had started a few days ago to move the memdup_user or
copy_from_user from the inidividual sockopts into sctp_setsockopt,
which is done with one patch per option, so it might suit Marcelo's
taste a bit better.  I did not start any work on getsockopt.

I'm not sure that 49 patches is actually any easier to review.
Most of the patches are just repetitions of the same change.
If they were in different files it might be different.

It's subjective, yes, but we hardly have patches over 5k lines.
In the case here, as changing the functions also requires changing
their call later on the file, it helps to be able to check that is was
properly updated. Ditto for chained functions.

Between them sparse and the compiler rather force you to find everything.
The main danger was failing to change sizeof(param) to sizeof(*param)
and I double-checked all the relevant lines/

...

What if you two work on a joint patchset for this? The proposals are
quite close. The differences around the setsockopt handling are
minimal already. It is basically variable naming, indentation and one
or another small change like:

If the changes match then the subfunctions are probably fine.

Because I've got at least 64 bytes I can convert in-situ and assume
(in getsockopt()) that I can action the request (if it only only a read)
and check the length later.
With only a memdup_user() you can't make those changes.

I was trying not to make extra changes.
(Apart from error path ones.)
Clearly that should be:
	sp->autoclose = min(*optval, net->sctp.max_autoclose);
But that requires additional thought.

quoted

If you try to do getsockopt() the same way it will be much
more complicated - you have to know whether the called function
did the copy_to_user() and then suppress it.

If it is not possible, then the setsockopt one already splited half of
the lines of the patch. :-)

Apart from the getsockopt() that is really a setsockopt() (CONNECTX3).
That might tie you in real knots.

	David

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