From: Marcelo Ricardo Leitner > Sent: 22 May 2020 15:36 > > On Fri, May 22, 2020 at 08:02:09AM +0000, David Laight wrote: > > From: Christoph Hellwig > > > 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. > From Christoph's to David's: > @@ -2249,11 +2248,11 @@ static int sctp_setsockopt_autoclose(struct sock *sk, u32 *autoclose, > return -EOPNOTSUPP; > if (optlen != sizeof(int)) > return -EINVAL; > - > - if (*autoclose > net->sctp.max_autoclose) > + > + sp->autoclose = *optval; > + > + if (sp->autoclose > net->sctp.max_autoclose) > sp->autoclose = net->sctp.max_autoclose; > - else > - sp->autoclose = *autoclose; 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. > > 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)
