Re: do a single memdup_user in sctp_setsockopt
From: 'Christoph Hellwig' <hch@lst.de>
Date: 2020-05-23 07:19:34
Also in:
linux-sctp
On Fri, May 22, 2020 at 11:36:23AM -0300, Marcelo Ricardo Leitner wrote:
quoted hunk ↗ jump to hunk
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. For example, I can spot things like this easier (from [PATCH 26/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_key)@@ -3646,7 +3641,6 @@ static int sctp_setsockopt_auth_key(struct sock *sk, } out: - kzfree(authkey); return ret; }...@@ -4771,7 +4765,10 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname, } release_sock(sk); - kfree(kopt); + if (optname == SCTP_AUTH_KEY) + kzfree(kopt); + else + kfree(kopt); out_nounlock: return retval;these are 1k lines apart. Yet, your implementation around this is better:@@ -3733,7 +3624,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk, } out: - kzfree(authkey); + memset(authkey, 0, optlen); return ret; }so that sctp_setsockopt() doesn't have to handle it specially.
Actually that implementation is wrong, if you want to move to a plain kfree it would have to be a memzero_explicit.
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:
I don't really want to waste too much time on this, as what I really need is to get the kernel_setsockopt removal series in ASAP. I'm happy to respin this once or twice with clear maintainer guidance (like the memzero_explicit), but I have no idea what you even meant with your other example or naming. Tell me what exact changes you want, and I can do a quick spin, but I don't really want a huge open ended discussion on how to paint the bikeshed.. Alternatively I'll also happily only do a partial conversion for what I need for the kernel_setsockopt removal and let you and Dave decided what you guys prefer for the rest.