( On Sun, Apr 15, 2018 at 8:56 AM, Eric W. Biederman <ebied...@xmission.com> wrote: > > Would you consider the patchset below for -rc2?
Ugh. I have an irrational dislike of "clear_siginfo()". It's a nasty broken interface, which just mis-spells "memset()", and makes it non-obvious that you could clear it other ways too (eg "kzalloc()" or whatever). And this series brings in a lot of new users. Honestly, both "clear_siginfo()" and "copy_siginfo()" are crazy interfaces. They may have made sense when converting code, but not so much now. If we want to have a function that initializes a siginfo, I think it should _actually_ initialize it, and at least take the two fields that a siginfo has to have to be valid: si_signo and si_code. So I'd rather see a patch that does something like this: - clear_siginfo(info); - info->si_signo = sig; - info->si_errno = 0; - info->si_code = SI_USER; - info->si_pid = 0; - info->si_uid = 0; + init_siginfo(info, sig, SI_USER); which at least makes that function be *worth* something, not just a bad spelling of memset. It's not just the removal of pointless "set to zero", it's the whole concept of "this function actually makes a valid siginfo", which is lacking in the existing function. (Yeah, yeah, si_errno is "generic" too and always part of a siginfo, but nobody cares. It's pretty much always set to zero, it would be stupid to add that interface to the "init_siginfo()" function. So just clearing it is fine, the one or two places that want to set it to some silly value can do it themselves). Then your series would incidentally also: (a) make for fewer lines overall, rather than add lines (b) make it clear that we always initialize si_code, which now *must* be a valid value with all the recent siginfo changes. Hmm? The other thing we should do is to get rid of the stupid padding. Right now "struct siginfo" is pointlessly padded to 128 bytes. That is completely insane, when it's always just zero in the kernel. So put that _pad thing inside #ifndef __KERNEL__, and make copy_siginfo_to_user() write the padding zeroes when copying to user space. The reason for the padding is "future expansion", so we do want to tell the user space that it's maybe up to 128 bytes in size, but if we don't fill it all, we shouldn't waste time and memory on clearing the padding internally. I'm certainly *hoping* nobody depends on the whole 128 bytes in rt_sigqueueinfo(). In theory you can fill it all (as long as si_code is negative), but the man-page only says "si_value", and the compat function doesn't copy any more than that either, so any user that tries to fill in more than si_value is already broken. In fact, it might even be worth enforcing that in rt_sigqueueinfo(), just to see if anybody plays any games.. On x86-64, without the pointless padding, the size of 'struct siginfo' inside the kernel would be 48 bytes. That's quite a big difference for something that is often allocated on the kernel stack. So I'm certainly willing to make those kinds of changes, but let's make them real *improvements* now, ok? Wasn't that the point of all the cleanups in the end? Linus